From 92cd69a68c673c703a602d8a7229491de917cebb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 02:09:34 +0000 Subject: [PATCH] fix(driver-sql): run speculative sequence statements under a savepoint (#8269) --- .changeset/autonumber-cold-race-savepoint.md | 58 +++++ .../sql-driver-autonumber-cold-race.test.ts | 199 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 98 +++++++-- 3 files changed, 340 insertions(+), 15 deletions(-) create mode 100644 .changeset/autonumber-cold-race-savepoint.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-autonumber-cold-race.test.ts diff --git a/.changeset/autonumber-cold-race-savepoint.md b/.changeset/autonumber-cold-race-savepoint.md new file mode 100644 index 0000000000..300c46d865 --- /dev/null +++ b/.changeset/autonumber-cold-race-savepoint.md @@ -0,0 +1,58 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): the first concurrent autonumber insert from two tenants no longer fails on Postgres with `25P02` (#8269) + +On Postgres, two tenants inserting into the same autonumber-bearing object **for +the first time concurrently** failed the whole batch with `25P02 current +transaction is aborted, commands ignored until end of transaction block`. The +counters advanced anyway, so the numbers that attempt had reserved were lost — +a permanent gap at the start of both tenants' sequences. The user-visible story +was *"creating the first records failed; I retried, it worked, and my numbering +starts at 0004."* + +`getNextSequenceValue` handled the first-insert race the way the idiom is +usually written — catch the unique violation, then recover **on the same +transaction**: + +```ts +try { + await trx(SEQUENCES_TABLE).insert({ ...insertRow, last_value: initial }); +} catch (err) { + existing = await trx(SEQUENCES_TABLE).where(key).forUpdate().first(); // 25P02 +} +``` + +On Postgres any statement error aborts the entire transaction, so the recovery +`SELECT … FOR UPDATE` **is** the statement that raises the error — the recovery +path could never run there. SQLite and MySQL do not abort on a statement error, +which is why the pattern looked correct, and why the SQLite-backed autonumber +suite could not catch it. + +Both speculative statements now run under a `SAVEPOINT`, released on success and +rolled back to on failure, so a failed attempt leaves the surrounding +transaction usable on every dialect. The race handler that was written for this +case now actually runs: the loser of the first-insert race blocks on the +winner's row, reads the committed counter, and takes its number from the UPDATE +path. + +**Scope of the second site.** The `SELECT … FOR UPDATE` fallback a few lines +above had the same shape and the same consequence. Its comment attributed it to +dialects that "reject `.forUpdate()` on a missing row" — measured on +`postgres:16`, that does not happen (a missing row returns zero rows), but the +catch is reachable for lock-level failures (deadlock `40P01`, lock/statement +timeout `55P03`/`57014`), and each of those was being masked as an +uninformative `25P02` by the fallback read. It is now under the same savepoint. + +**Not multi-org-only.** The report measured single-tenant bursts as safe; they +are only *flakier*. Measured before the fix, 5 rounds each of one tenant × N +cold concurrent inserts failed 0/5 (N=2), 1/5 (N=4), 3/5 (N=6) and 2/5 (N=12) +with the same `25P02`. Two tenants means two cold counter rows, which makes the +window near-certain to be hit rather than occasional — an amplifier, not a +precondition. Single-organization deployments were exposed too. + +Unchanged: what happens to numbers on a failed attempt. The reservation still +commits in its own transaction and is not rolled back with the caller's insert, +which is ordinary sequence semantics. SQLite and MySQL behaviour is unchanged — +the savepoint makes Postgres behave the way they already did. diff --git a/packages/drivers/driver-sql/src/sql-driver-autonumber-cold-race.test.ts b/packages/drivers/driver-sql/src/sql-driver-autonumber-cold-race.test.ts new file mode 100644 index 0000000000..65e5033ae7 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-autonumber-cold-race.test.ts @@ -0,0 +1,199 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8269 — the first CONCURRENT autonumber insert into a COLD object fails on + * Postgres with `25P02 current transaction is aborted`. + * + * `getNextSequenceValue` handled the first-insert race by catching the unique + * violation and recovering **inside the same transaction**. On Postgres any + * statement error aborts the whole transaction, so the recovery `SELECT … FOR + * UPDATE` was itself the statement that raised the error — the recovery path + * could never run there. The fix runs each speculative statement under a + * SAVEPOINT (`attemptWithoutPoisoning`). + * + * ## Why this file needs a live Postgres, and all four conditions + * + * The reporter measured that dropping any single condition hides the defect, + * which is why the (SQLite-backed, single-tenant, warm) autonumber suite never + * caught it in the first place: + * + * - **Postgres**: SQLite and MySQL do not abort a transaction on a statement + * error, so the buggy recovery path works there. SQLite is not a weaker + * version of this test — it is a test that cannot fail. + * - **cold**: a counter row that already exists takes the UPDATE path and + * never reaches the speculative INSERT. Warm bursts pass unfixed. + * - **concurrent**: serial writers never race the first INSERT. + * - **cross-tenant**: two tenants means two cold counter rows, which is what + * makes the race window near-certain to be hit rather than occasional. + * + * That last point is a REFINEMENT of the report, measured here on `postgres:16` + * before the fix: single-tenant cold bursts are not safe, only *flaky* — 5 + * rounds each of 1 tenant × N cold concurrent inserts failed 0/5 (N=2), 1/5 + * (N=4), 3/5 (N=6) and 2/5 (N=12) with the same `25P02`. The tenant boundary is + * not a precondition of the defect, only an amplifier, so single-org + * deployments were exposed too. The guard below is still written cross-tenant + * because that is the shape that fails deterministically. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import { DIALECT_CELLS, declareUnprovisionedCell, type DialectCell } from './live-dialect-matrix.testkit.js'; + +const TABLE = 'os8269_cold_race'; +const SEQUENCES_TABLE = '_objectstack_sequences'; + +const SHAPE = { + name: TABLE, + fields: { + organization_id: { type: 'string' }, + code: { type: 'autonumber', format: 'TK-{0000}' }, + name: { type: 'string' }, + }, +} as any; + +/** Rows of a raw result, across knex's three dialect shapes. */ +function rowsOf(res: any): any[] { + if (Array.isArray(res) && Array.isArray(res[0])) return res[0]; // mysql2: [rows, fields] + if (Array.isArray(res)) return res; // better-sqlite3 + return res?.rows ?? []; // pg +} + +function coldRaceSuite(cell: DialectCell, role: string) { + describe(`sql-driver — autonumber cold cross-tenant race (${cell.label}) [#8269]`, () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver(cell.config()); + // COLD is the whole point: drop the data table AND the counter rows, so + // every run starts with no sequence row for this object. + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver + .execute(`delete from ${SEQUENCES_TABLE} where "object" = '${TABLE}'`) + .catch(() => {}); + await driver.initObjects([SHAPE]); + }); + + afterEach(async () => { + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver + .execute(`delete from ${SEQUENCES_TABLE} where "object" = '${TABLE}'`) + .catch(() => {}); + await driver.disconnect(); + }); + + it(`issues a contiguous band per tenant on a cold concurrent cross-tenant burst (${role})`, async () => { + const tenants = ['os8269_orgA', 'os8269_orgB']; + const per = 6; + + // One burst, both tenants interleaved, nothing warmed up first. Unfixed, + // this rejects with 25P02 on Postgres. + const rows = await Promise.all( + tenants.flatMap((t) => + Array.from({ length: per }, (_, i) => + driver.create(TABLE, { organization_id: t, name: `${t}-${i}` }), + ), + ), + ); + + expect(rows).toHaveLength(tenants.length * per); + for (const t of tenants) { + const issued = rows + .filter((r: any) => r.organization_id === t) + .map((r: any) => r.code) + .sort(); + // Each tenant gets its own counter, so each band starts at 1 and is + // contiguous — no duplicates, and no numbers burned by a failed attempt + // (the permanent gap the report saw after retrying a failed burst). + expect(issued).toEqual( + Array.from({ length: per }, (_, i) => `TK-${String(i + 1).padStart(4, '0')}`), + ); + } + }); + + it(`advances each tenant's counter exactly once per issued number (${role})`, async () => { + const tenants = ['os8269_orgC', 'os8269_orgD']; + const per = 4; + await Promise.all( + tenants.flatMap((t) => + Array.from({ length: per }, (_, i) => + driver.create(TABLE, { organization_id: t, name: `${t}-${i}` }), + ), + ), + ); + + const counters = rowsOf( + await driver.execute( + `select tenant_id, last_value from ${SEQUENCES_TABLE} where "object" = '${TABLE}' order by tenant_id`, + ), + ); + // `last_value` is a bigint — pg returns it as a string. + expect(counters.map((r: any) => [String(r.tenant_id), Number(r.last_value)])).toEqual( + tenants.map((t) => [t, per]), + ); + }); + }); +} + +const pgCell = DIALECT_CELLS.find((c) => c.id === 'pg')!; +if (pgCell.available) { + // THE regression guard. Postgres is the only dialect that aborts a + // transaction on a statement error, so it is the only one that can fail. + coldRaceSuite(pgCell, 'regression guard'); +} else { + declareUnprovisionedCell(pgCell, 'autonumber cold cross-tenant race'); +} + +// A control, not coverage: SQLite passed this before the fix (the report +// measured it) and must still pass after, which is what pins "the savepoint +// changed nothing on the dialects that were already correct". Deleting the +// Postgres cell above and leaving this one would be a suite that cannot fail. +coldRaceSuite( + DIALECT_CELLS.find((c) => c.id === 'sqlite')!, + 'unaffected control — passes before AND after the fix', +); + +describe(`sql-driver — attemptWithoutPoisoning (${pgCell.available ? 'live postgres' : 'skipped'}) [#8269]`, () => { + // The mechanism itself, pinned directly: this is what makes the SECOND + // speculative site (`SELECT … FOR UPDATE`, which has no `ON CONFLICT` form) + // safe as well. Postgres-only for the same reason as above. + it.skipIf(!pgCell.available)( + 'leaves the surrounding transaction usable after a statement error', + async () => { + const driver = new SqlDriver(pgCell.config()); + const probe = 'os8269_poison_probe'; + try { + const knex = (driver as any).knex; + await knex.schema.dropTableIfExists(probe); + await knex.schema.createTable(probe, (t: any) => { + t.string('k').primary(); + t.integer('v'); + }); + + await knex.transaction(async (trx: any) => { + await trx(probe).insert({ k: 'a', v: 1 }); + + const failed = await (driver as any).attemptWithoutPoisoning(trx, (scoped: any) => + scoped(probe).insert({ k: 'a', v: 2 }), + ); + // The original error is preserved, not swallowed — the caller needs it + // to tell "another writer raced me" from anything else. + expect(failed.ok).toBe(false); + expect((failed.error as any).code).toBe('23505'); + + // Unfixed, this read is where 25P02 surfaced. + const row = await trx(probe).where({ k: 'a' }).forUpdate().first(); + expect(Number(row.v)).toBe(1); + + const ok = await (driver as any).attemptWithoutPoisoning(trx, (scoped: any) => + scoped(probe).insert({ k: 'b', v: 3 }), + ); + expect(ok.ok).toBe(true); + }); + + await knex.schema.dropTableIfExists(probe); + } finally { + await driver.disconnect(); + } + }, + ); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 49ed7028ad..a4c7a22cb2 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -4261,6 +4261,66 @@ export class SqlDriver implements IDataDriver { return maxN; } + /** + * Run a SPECULATIVE statement — one whose failure is an expected outcome the + * caller recovers from — so that its error cannot poison the surrounding + * transaction. + * + * # Why this exists (#8269) + * + * On Postgres, ANY statement error aborts the whole transaction: every + * subsequent statement returns `25P02 current transaction is aborted` until + * rollback. So the `try { … } catch { …recover… }` idiom, where the recovery + * itself issues SQL on the same transaction, can never run there — the + * recovery statement is the one that raises the error you observe. SQLite and + * MySQL do not abort the transaction on a statement error, which is why the + * idiom looked correct and why the SQLite-backed autonumber suite never + * caught it. + * + * Measured on `postgres:16`, two tenants inserting into the same cold + * autonumber object concurrently: `select … from "_objectstack_sequences" + * where "key_hash" = $1 limit $2 for update - current transaction is aborted, + * commands ignored until end of transaction block`. The whole batch failed + * while the counters advanced, so the reserved numbers were lost — a + * permanent gap at the start of both tenants' sequences. + * + * A knex nested transaction is a `SAVEPOINT`, released on success and rolled + * back to on failure, so the outer transaction stays usable on every dialect + * (verified: PG 16, and better-sqlite3 at pool `max: 1`, where the savepoint + * rides the parent's connection and never asks the pool for a second one — + * the deadlock #4250 exists to prevent). + * + * Chosen over `INSERT … ON CONFLICT DO NOTHING` on two pieces of evidence: + * + * 1. It is the only form that covers BOTH speculative sites here — the other + * one is a `SELECT … FOR UPDATE`, and there is no `ON CONFLICT` for a + * read. + * 2. `ON CONFLICT (object, tenant_id, field)` — the columns the legacy key + * shape uses when `sequencesHasKeyHash` is false — raises `42P10 there is + * no unique or exclusion constraint matching the ON CONFLICT + * specification` against an INTERIM table whose primary key is the FOUR + * columns `(object, tenant_id, field, scope)`. That pairing is reachable: + * it is exactly what a failed {@link ensureSequencesKeyHashShape} + * migration leaves behind. It would have replaced this bug with a harder + * one on the deployments least able to absorb it. + * + * Returns a discriminated result rather than rethrowing, so the caller keeps + * the ORIGINAL error to inspect or rethrow — an `ON CONFLICT DO NOTHING` that + * silently affects zero rows cannot tell "another writer raced me" apart from + * "the row was rejected for some other reason". + */ + protected async attemptWithoutPoisoning( + trx: Knex.Transaction, + attempt: (scoped: Knex.Transaction) => Promise | Knex.QueryBuilder, + ): Promise<{ ok: true; value: T } | { ok: false; error: unknown }> { + try { + const value = await trx.transaction(async (scoped) => attempt(scoped)); + return { ok: true, value: value as T }; + } catch (error) { + return { ok: false, error }; + } + } + /** * Atomically reserve and return the next sequence value for * `(object, tenantId, field)`. Bootstraps from the data-table MAX on @@ -4349,13 +4409,21 @@ export class SqlDriver implements IDataDriver { return runner.transaction(async (trx) => { // Lock the row (no-op on SQLite, real lock on Postgres/MySQL). + // + // Speculative, so it runs under a savepoint: `.forUpdate()` on a MISSING + // row does NOT throw on Postgres (measured: returns zero rows), so the + // fallback below is unreachable for the reason the old comment gave. It + // IS reachable for lock-level failures — deadlock (40P01), lock/statement + // timeout (55P03/57014) — and without the savepoint each of those turned + // into a `25P02` from the fallback SELECT, masking the real error. let existing: any; - try { - existing = await trx(SEQUENCES_TABLE).where(key).forUpdate().first(); - } catch { - // Some dialects/versions reject .forUpdate() on a missing row in - // weird ways; fall back to plain SELECT then rely on transaction - // isolation. Postgres/MySQL behave normally here. + const locked = await this.attemptWithoutPoisoning(trx, (scoped) => + scoped(SEQUENCES_TABLE).where(key).forUpdate().first(), + ); + if (locked.ok) { + existing = locked.value; + } else { + // Fall back to a plain SELECT then rely on transaction isolation. existing = await trx(SEQUENCES_TABLE).where(key).first(); } @@ -4370,15 +4438,15 @@ export class SqlDriver implements IDataDriver { suffix, ); const initial = seedMax + 1; - try { - await trx(SEQUENCES_TABLE).insert({ ...insertRow, last_value: initial }); - return initial; - } catch (err) { - // Another writer raced us to the first INSERT. Fall through to - // the UPDATE path with the now-present row. - existing = await trx(SEQUENCES_TABLE).where(key).forUpdate().first(); - if (!existing) throw err; - } + const inserted = await this.attemptWithoutPoisoning(trx, (scoped) => + scoped(SEQUENCES_TABLE).insert({ ...insertRow, last_value: initial }), + ); + if (inserted.ok) return initial; + // Another writer raced us to the first INSERT. Fall through to + // the UPDATE path with the now-present row. This `SELECT … FOR UPDATE` + // blocks until the winner commits, so it sees the committed counter. + existing = await trx(SEQUENCES_TABLE).where(key).forUpdate().first(); + if (!existing) throw inserted.error; } const next = Number(existing.last_value) + 1;