Skip to content

fix(driver-sql): the first concurrent autonumber insert from two tenants no longer fails on Postgres with 25P02 - #8279

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-8269-autonumber-cold-race-25p02
Aug 13, 2026
Merged

fix(driver-sql): the first concurrent autonumber insert from two tenants no longer fails on Postgres with 25P02#8279
os-zhuang merged 1 commit into
mainfrom
claude/issue-8269-autonumber-cold-race-25p02

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes#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 reserved were lost — a permanent gap at the start of both tenants' sequences.

Cause

getNextSequenceValue handled the first-insert race by catching the unique violation and recovering on the same transaction. On Postgres any statement error aborts the entire transaction, so the recovery SELECT … FOR UPDATEis the statement that raises the error the report observed — the recovery path could never run there. SQLite and MySQL do not abort a transaction on a statement error, which is why the idiom looked correct and why the SQLite-backed autonumber suite could not catch it.

Fix

Both speculative statements now run under a SAVEPOINT (attemptWithoutPoisoning, a knex nested transaction), 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 blocks on the winner's row, reads the committed counter, and takes its number from the UPDATE path.

Both catch-inside-transaction sites are fixed. The SELECT … FOR UPDATE fallback above the measured site 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), so the catch was unreachable for the documented reason. It 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. Same savepoint, so the fallback runs on a clean transaction.

Why savepoint over INSERT … ON CONFLICT DO NOTHING

Both candidates were measured; ON CONFLICT lost on evidence:

  1. It cannot cover the second site — that one is a SELECT … FOR UPDATE, and there is no ON CONFLICT for a read.
  2. It does not compose with both key shapes. ON CONFLICT (object, tenant_id, field) — the columns the legacy key 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 ensureSequencesKeyHashShape migration leaves behind. It would have replaced this bug with a harder one on the deployments least able to absorb it.

The helper also returns a discriminated result rather than rethrowing, so the caller keeps the original error — an ON CONFLICT DO NOTHING affecting zero rows cannot tell "another writer raced me" apart from "the row was rejected for another reason".

Refinement to the report: this is not multi-org-only

The report measured single-tenant bursts as safe. They are only flakier. Measured on postgres:16 before the fix, 5 rounds each of one tenant x N cold concurrent inserts:

caserounds failedcode
1 tenant x2 cold0/5
1 tenant x4 cold1/525P02
1 tenant x6 cold3/525P02
1 tenant x12 cold2/525P02

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. The regression guard is still written cross-tenant because that is the shape that fails deterministically.

Tests

sql-driver-autonumber-cold-race.test.ts, reusing the existing live-dialect harness (live-dialect-matrix.testkit.ts + OS_TEST_POSTGRES_URL, the Temporal Conformance (live PG + MySQL) job's mechanism) — no second Postgres mechanism invented. The guard is Postgres-backed AND cross-tenant AND cold AND concurrent, all four; an unprovisioned pg cell is a reported skip that becomes a hard failure under OS_EXPECT_LIVE_DIALECT_MATRIX=1, so CI cannot silently degrade it. A SQLite cell runs the same burst as an explicitly-labelled control (it passed before the fix and must still pass), not as coverage.

Reverse verification, prediction stated before running — revert sql-driver.ts only, keep the tests:

× issues a contiguous band per tenant on a cold concurrent cross-tenant burst (regression guard)
× advances each tenant's counter exactly once per issued number (regression guard)
× leaves the surrounding transaction usable after a statement error
-> TypeError: driver.attemptWithoutPoisoning is not a function
-> error: select * from "_objectstack_sequences" where "key_hash" = $1 limit $2 for update
- current transaction is aborted, commands ignored until end of transaction block
✓ ...(sqlite) issues a contiguous band per tenant... (unaffected control)
✓ ...(sqlite) advances each tenant's counter exactly once... (unaffected control)
Tests 3 failed | 2 passed (5)

The two SQLite rows staying green is the load-bearing half: it confirms the control is genuinely a control rather than a second copy of the guard.

Full @objectstack/driver-sql suite against live Postgres 16: 91 files passed, 1629 tests passed, 0 failed; typecheck clean.

Deliberately unchanged

What happens to numbers on a failed attempt. The reservation still commits in its own transaction (runner.transaction, parentTrx ?? this.knex) and is not rolled back with the caller's insert — ordinary sequence semantics, out of scope here. No behaviour change for SQLite or MySQL: the savepoint makes Postgres behave the way those two already did.


Generated by Claude Code

@vercel

vercelBot commented Aug 13, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 13, 2026 2:18am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql.

8 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via @objectstack/driver-sql)
  • content/docs/getting-started/glossary.mdx(via @objectstack/driver-sql)
  • content/docs/kernel/services-checklist.mdx(via @objectstack/driver-sql)
  • content/docs/plugins/anatomy.mdx(via @objectstack/driver-sql)
  • content/docs/plugins/packages.mdx(via @objectstack/driver-sql)
  • content/docs/protocol/kernel/index.mdx(via @objectstack/driver-sql)
  • content/docs/protocol/kernel/lifecycle.mdx(via @objectstack/driver-sql)
  • content/docs/protocol/objectql/query-syntax.mdx(via @objectstack/driver-sql)

1 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/driver-sql)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 13, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 13, 2026 02:35
@os-zhuang
os-zhuang added this pull request to the merge queueAug 13, 2026
Merged via the queue into main with commit ecb39eaAug 13, 2026
26 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-8269-autonumber-cold-race-25p02 branch August 13, 2026 02:53
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

2 participants

@os-zhuang@claude