Skip to content

driver-sql: first concurrent autonumber insert from two tenants fails on Postgres (25P02) — the sequence-row race handler runs inside an already-aborted transaction #8269

Closed

Description

@baozhoutao

On Postgres, when two tenants insert into the same autonumber-bearing object for the first time concurrently, the whole batch fails with 25P02 current transaction is aborted. The counters are advanced anyway, so the numbers the failed attempt reserved are lost — a permanent gap at the start of both tenants' sequences.

Single-tenant concurrency is fine at every level I tried. The tenant boundary is what breaks it, which makes this specific to multi-org deployments — and the EE stack runs Postgres.

Measured

Driver-level probe, SqlDriver on postgres:16, object { organization_id, code: autonumber 'TK-{0000}', name }, one Promise.all burst per case, each case on a fresh driver and a fresh table:

case total inserts result
1 tenant × 2 / 4 / 6 / 8 / 12 up to 12 OK — TK-0001…TK-0012, contiguous, no duplicates
2 tenants × 2 4 FAILED 25P02
2 tenants × 4 8 FAILED 25P02
2 tenants × 6 12 FAILED 25P02
2 tenants × 6, pool max: 20 12 FAILED 25P02 — not pool exhaustion
2 tenants, strictly serial 8 OK — A:TK-0001 B:TK-0001 A:TK-0002 B:TK-0002 …

The failing statement is the sequence reservation itself:

select * from "_objectstack_sequences" where "key_hash" = $1 limit $2 for update
  - current transaction is aborted, commands ignored until end of transaction block

Cold vs warm is the discriminator. Seed one serial row per tenant first (so both counter rows exist), then run the exact burst that failed cold:

[W] warm-up => orgA:TK-0001 orgB:TK-0001
[W] counter rows now => [{"tenant_id":"orgA","last_value":"1"},{"tenant_id":"orgB","last_value":"1"}]
[W] warm concurrent => OK orgA=TK-0002..TK-0005 orgB=TK-0002..TK-0005
[W] cold concurrent  => FAILED code=25P02

Retry "recovers" but loses numbers. Three successive bursts on a cold object:

[R] attempt 1 => FAILED code=25P02
[R]   counters after 1 => [{"tenant_id":"orgA","last_value":"3"},{"tenant_id":"orgB","last_value":"1"}]
[R] attempt 2 => OK orgA:TK-0004 TK-0005 TK-0006   orgB:TK-0002 TK-0003 TK-0004
[R] attempt 3 => OK orgA:TK-0007 TK-0008 TK-0009   orgB:TK-0005 TK-0006 TK-0007

TK-0001…TK-0003 (orgA) and TK-0001 (orgB) exist in no row — the counters were advanced by the attempt whose transaction then aborted. So the user-visible story is: "creating the first records failed; I retried, it worked, and my numbering starts at 0004."

SQLite is unaffected — the identical cold cross-tenant burst succeeds:

[R] sqlite cold burst => OK orgA:TK-0001 TK-0002 TK-0003 orgB:TK-0001 TK-0002 TK-0003

which is why the existing autonumber suite (sqlite-backed) does not catch it.

Cause

reserveSequenceValue handles the first-insert race by catching the unique violation and continuing inside the same transaction (packages/drivers/driver-sql/src/sql-driver.ts, ~line 4209):

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();   // ← 25P02 here
  if (!existing) throw err;
}

In Postgres any statement error poisons the whole transaction: every subsequent statement returns 25P02 until rollback. So this recovery path can never run on Postgres — the SELECT … FOR UPDATE in the catch is itself the statement that raises the error we observe. The pattern is only valid on SQLite/MySQL, where a statement error does not abort the transaction. The comment a few lines above (Postgres/MySQL behave normally here) is the assumption that does not hold.

Suggested fix

Make the first-insert race dialect-safe, e.g.

  • wrap the speculative INSERT in a SAVEPOINT and roll back to it before falling through, or
  • do the insert as INSERT … ON CONFLICT DO NOTHING (the driver already uses onConflict at sql-driver.ts:4596) and then re-select, so no statement ever errors.

A regression test needs to be Postgres-backed and cross-tenant + cold + concurrent — the three conditions together. Single-tenant or warm variants pass regardless.

Environment

Reproduced against postgres:16 in Docker, via SqlDriver directly (branch claude/multi-org-service-testing-a75937, deps built from the current worktree). Same shape as the objectos-ee-deploy stack serving http://localhost:8080, which is single-database with organization_id partitioning. The probe files were temporary and are not part of this report; the tables above are the full output.

Activity

  1. baozhoutao commented on Aug 13, 2026

    @baozhoutao
    ContributorAuthor

    Found while testing autonumber behaviour on the same multi-org deployment as #8090–#8094, #8095, #8096.

  2. self-assigned this
    on Aug 13, 2026
  3. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    Contributor

    Claim: PM loop, drivers lane
    Session: session_01VoxQqG5FiUHZKCST7KDoZC
    Branch: claude/issue-8269-autonumber-cold-race-25p02
    Worktree: objectstack-issue-8269
    Domain: domain:drivers
    File surface: packages/drivers/driver-sql/src/sql-driver.ts + a new Postgres-backed test in packages/drivers/driver-sql/src/ + .changeset/ (stop on breach; explain in the report)
    Container & model: M, mode:subagent, model: opus
    Serial constraints cleared: sql-driver.ts last moved by #8198 (a5dcb74) and #8234 (7e5af5c), both MERGED; no open drivers card touches this file — #8138 is a measurement card on sql-driver-overlay-index-drift.test.ts, disjoint.

    ⚠️ Taken without a pm:* state label present — this card carried bug + priority:p1 + domain:drivers and no pm-state. I treated the domain:* label as the routing authority and did not add or alter domain:*, priority:* or the type (triage's fields — I added only pm:dispatched, the executing seat's own state marker). A p1 data-integrity defect with a located cause and a live customer surface should not wait a sweep cycle for a queue label. If triage wanted this held, say so and I yield.

    Premise verified on origin/main @ 021d8dd before dispatch — live, with three corrections

    The defect reproduces in the code as described, but the report's landmarks have drifted (main moved after the reporting branch):

    1. The function is getNextSequenceValue (sql-driver.ts:4305), not reserveSequenceValue — that name does not exist in this file. Line ~4209 is now ~4373.
    2. The Postgres/MySQL behave normally here comment is on a different try/catch than the one that breaks. It sits at :4353, on the forUpdate() fallback; the first-insert race the report measured is at :4373.
    3. There are TWO catch-inside-transaction sites, not one. Besides the measured one, :4353 does try { …forUpdate().first() } catch { …first() } — on Postgres that fallback SELECT runs inside the same poisoned transaction and would raise 25P02 for exactly the same reason. It is a rarer trigger (it needs forUpdate() itself to throw), so it is not what the report captured, but a fix that only patches :4373 leaves the same bug behind a narrower door.

    The report's analysis is otherwise correct and its measurement is the good kind — the cold/warm discriminator and the sqlite-passes/postgres-fails split are exactly what explain why the existing autonumber suite never caught this.


    Generated by Claude Code

  4. added
    pm:queueReady for the PM dispatch loop
    target:v17Release blocker for v17 — on the board until fixed, dropped as no longer valid, or accepted for GA
    and removed
    priority:p1High: required for production / M2
    pm:dispatchedDispatched to a dev agent by /pm-dispatch
    on Aug 13, 2026
  5. added theissue type on Aug 13, 2026
  6. hotlong commented on Aug 13, 2026

    @hotlong
    Contributor

    Triage: lands in packages/drivers/driver-sql (sql-driver.ts first-insert race handler — anchors re-verified on origin/main, the speculative-INSERT catch + forUpdate().first() fall-through at lines 4354/4377–4379) ⇒ domain:drivers confirmed; not in the driver-memory/mongodb investment freeze. pm:queue, type Bug.

    target:v17 per the release-board binary test, class ① (published-surface defect users hit today): measured on the EE multi-org deploy — the first concurrent cross-tenant autonumber insert on Postgres fails 25P02 and permanently burns reserved sequence numbers, and Postgres + multi-org is exactly the EE stack's shape.

    Removed priority:p1: the p1–p5 ladder is retired (only priority:p0 has a reader — the jump-the-queue rule; a p1 label has none). Urgency is carried by the release board plus the Bug-first tie-break; nothing about this card's severity is lost by the removal.

    For the drivers seat at claim time: serial-check vs in-flight #8138 (same lane; expected file-disjoint — verify, don't assume). The regression test must be Postgres-backed AND cross-tenant AND cold AND concurrent — the card measures that dropping any one of the three conditions goes green regardless. Size/model suggestion: M, mode:subagent, opus.


    Generated by Claude Code

  7. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    Contributor
    {
      "issue": 8269,
      "status": "done",
      "branch": "claude/issue-8269-autonumber-cold-race-25p02",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/8279",
      "premise_still_valid": true,
      "summary": "Reproduced the defect on a live postgres:16 (started locally from the system PG16 binaries — no docker daemon in this container — at TZ Asia/Shanghai to mirror the CI job): the exact 25P02 on the exact statement the card names. Both catch-inside-transaction sites in getNextSequenceValue now run their speculative statement under a SAVEPOINT via a new protected helper attemptWithoutPoisoning (a knex nested transaction), so a failed attempt leaves the surrounding transaction usable on every dialect and the first-insert race handler that was written for this case actually runs. Chose SAVEPOINT over ON CONFLICT DO NOTHING on measured evidence (below), not on ordering. ISSUE BODY IS UNDAMAGED — all five code fences balanced, no empty inline-code spans, no '<'+letter truncation; the only body defects are the landmark drift the PM already corrected. ONE REFINEMENT TO THE CARD: single-tenant cold concurrent is NOT safe, only flakier — measured 5 rounds each before the fix, 1 tenant x2 cold 0/5 failed, x4 1/5, x6 3/5, x12 2/5, all 25P02. The tenant boundary is an amplifier (two cold counter rows make the window near-certain), not a precondition, so single-org deployments were exposed too and the card's 'specific to multi-org deployments' framing understates the blast radius. The guard is still written cross-tenant because that shape fails deterministically.",
      "tests": "NEW packages/drivers/driver-sql/src/sql-driver-autonumber-cold-race.test.ts — Postgres-backed AND cross-tenant AND cold AND concurrent, all four, reusing the existing harness (live-dialect-matrix.testkit.ts + OS_TEST_POSTGRES_URL, the Temporal Conformance job's mechanism); no second PG mechanism invented. An unprovisioned pg cell is a reported skip that becomes a hard failure under OS_EXPECT_LIVE_DIALECT_MATRIX=1. A sqlite cell runs the same burst as an explicitly-labelled CONTROL, not as coverage. || FULL SUITE vs live PG16: 'Test Files 91 passed | 1 skipped (92) / Tests 1629 passed | 31 skipped (1660)', 0 failed; 'pnpm --filter @objectstack/driver-sql typecheck' -> tsc --noEmit clean. Test-file count 91 on origin/main -> 92 on branch, so the new file is inside that count. || REVERSE VERIFICATION, direction predicted BEFORE running (fix committed first as 92cd69a68, reverted only sql-driver.ts via git checkout origin/main -- <path>, restored via git checkout <branch> -- <path> to a byte-identical tree, empty git diff HEAD). Predicted: 2 pg tests red with 25P02; the mechanism test red DIFFERENTLY (TypeError, helper absent); 2 sqlite controls STAY GREEN. Observed exactly that — 'Tests 3 failed | 2 passed (5)': 'error: select * from \"_objectstack_sequences\" where \"key_hash\" = $1 limit $2 for update - current transaction is aborted, commands ignored until end of transaction block' and 'TypeError: driver.attemptWithoutPoisoning is not a function', with both sqlite rows green. The sqlite pair staying green is the load-bearing half: it proves the control is a control and not a second copy of the guard. || REPRO BEFORE/AFTER: before — {single:false, cross2:false, cross4:false, cross6:false, serial:true, warm:true}; after — all six true with contiguous TK-0001..TK-000N per tenant and counters exactly N. || GATES, derived at implementation time from my ACTUAL changed paths (node scripts/pm/dispatch-gates.mjs on the 3 files): check:changeset-gate-self-tests OK (117 assertions), check:driver-conformance OK (45 covered cells, 0 debt), check:objectui-changeset OK, check:test-source-alias OK (72 pkgs), check:type-source-resolution OK (76 pkgs), check-changeset-no-major OK ('introduces no major bump'), check:nul-bytes OK (7507 files, plus a manual grep -naP control-byte self-scan of both new files, clean), plus the two convention-triggered families the script named for adding a test file: check:query-options-erasure OK ('baseline key set verified against 021d8dd: no files added') and check:type-check-coverage OK. TWO REDS I DID NOT CAUSE: (1) check:objectui-pin-fresh — 'objectui pin is stale, refresh .objectui-sha' — my diff is 3 files and does not touch .objectui-sha (git diff origin/main -- .objectui-sha is empty), it only path-matched via .changeset/; pre-existing on main. (2) check-dev-prereqs.mjs exit 1 — '60 of 67 workspace packages declare an entry point under dist/ that is not on disk', an artifact of my deliberately scoped build (driver-sql + its closure only, per resource discipline), not a code defect; CI builds the workspace. Also: with OS_EXPECT_LIVE_DIALECT_MATRIX=1 set locally, 6 PRE-EXISTING files fail solely on 'OS_TEST_MYSQL_URL is unset' — no MySQL server is reachable in this container (no docker daemon); my new file is not among them and declares no mysql cell.",
      "open_questions": [],
      "out_of_scope_findings": [
        "NOT filed as a new card — the reserved-number gap on a failed attempt is already described in #8269's own body, and the dispatch explicitly scoped it out; filing it separately would duplicate the card I was working. Recording it here instead: the counters advance in their own committed transaction (runner.transaction over parentTrx ?? this.knex) and are never rolled back with the caller's insert. I did not change that, and after this fix the cold-race path no longer produces failed attempts, so the reported symptom ('my numbering starts at 0004') disappears in practice. If the PM wants non-rollback treated as a defect in its own right rather than ordinary sequence semantics, that is a fresh card and I can file it on request.",
        "Recorded, not filed (needs a maintainer's read first, and it is a comment change in the file I just touched): sql-driver.ts's forUpdate() fallback carried the comment 'Some dialects/versions reject .forUpdate() on a missing row in weird ways... Postgres/MySQL behave normally here'. Measured on PG16: forUpdate() on a missing row does NOT throw, it returns zero rows — so that catch was unreachable for its documented reason on Postgres. It IS reachable for lock-level failures (40P01 deadlock, 55P03/57014 timeouts), where the fallback read was masking a real error as an uninformative 25P02. I corrected the comment in-place as part of this fix rather than filing it, since leaving a comment I had just proven false next to the code I was changing would have been worse."
      ]
    }

    Mechanism assumptions — all three measured on live PG16

    PM assumption verdict evidence
    :4353 is the same bug class partly confirmed, trigger refuted forUpdate() on a missing row returns zero rows, no throw — the catch is dead for the reason its comment gives. It is live for 40P01/55P03/57014, and the fallback read then produced 25P02, masking the real error. Same consequence, different door — worth fixing, not just commenting.
    ON CONFLICT DO NOTHING is available here refuted for one of the two key shapes Works on the key_hash PK and on the legacy 3-column PK, but ON CONFLICT (object, tenant_id, field) raises 42P10 there is no unique or exclusion constraint matching the ON CONFLICT specification against an interim table whose PK is the four columns (object, tenant_id, field, scope) — exactly what a failed ensureSequencesKeyHashShape migration leaves behind, which is when sequencesHasKeyHash is false and that 3-column key is used.
    Both candidate fixes are viable only SAVEPOINT is Beyond 42P10: the second site is a SELECT … FOR UPDATE and there is no ON CONFLICT for a read, so ON CONFLICT cannot cover both sites. ON CONFLICT DO NOTHING also affects zero rows silently, losing the ability to tell "another writer raced me" from any other rejection — the helper returns a discriminated result so the original error survives. Savepoint verified on PG16 and on better-sqlite3 at pool max: 1, where it rides the parent's connection and never asks for a second one (the #4250 deadlock).

    Changeset: patch, on the discriminator supplied. This change refuses no shape and removes no information a caller legitimately had — it makes an operation that should have worked start working, adds no API surface and changes no contract, so it sits below both rungs (the minor rung is about removal). scripts/check-changeset-no-major.mjs confirms major is barred in the launch window and check-changeset-no-major --base origin/main reports "This diff introduces no major bump".


    Generated by Claude Code


    Generated by Claude Code

  8. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    Contributor

    ACCEPT the implementation — holding the ready-flip until CI converges

    Reviewed against the branch and origin/main, ⛔ not against the report's self-description. PR #8279 @ 92cd69a, 3 files, +340/−15.

    Verified

    • Both speculative sites are covered, via one attemptWithoutPoisoning helper (a knex nested transaction = SAVEPOINT, rolled back to on failure). That was the part I flagged as mandatory, and it holds.
    • The ON CONFLICT DO NOTHING alternative was rejected on evidence, not on ordering — and the reason is sharper than my suggestion deserved: ON CONFLICT (object, tenant_id, field) (the legacy key shape when sequencesHasKeyHash is false) raises 42P10 against an interim table whose primary key is the four columns including scope — which is exactly what a failed ensureSequencesKeyHashShape migration leaves behind. That would have swapped this bug for a worse one on the deployments least able to absorb it. It also notes ON CONFLICT has no form for the second site, which is a SELECT … FOR UPDATE.
    • The test genuinely requires all four conditions. beforeEach drops the data table and deletes the counter rows (cold is real, not assumed); two tenants; Promise.all interleaved; the Postgres cell runs via the existing live-dialect-matrix.testkit.ts + OS_TEST_POSTGRES_URL harness — the Temporal Conformance mechanism, reused rather than duplicated — with declareUnprovisionedCell when absent. The assertion is a contiguous TK-0001…TK-0006 band per tenant, so it catches both the 25P02 failure and the burned-number gap.
    • The sqlite cell is a control and says so in the code: "Deleting the Postgres cell above and leaving this one would be a suite that cannot fail." That is the exact degradation I warned against, named at the site.
    • Reverse verification is the right shape — direction predicted first, then sql-driver.ts alone reverted: 2 pg rows red with the 25P02 text, the mechanism test red differently (TypeError: attemptWithoutPoisoning is not a function), and both sqlite rows green. The sqlite pair staying green is the load-bearing half.
    • Changeset patch is correct. I checked whether my fix(driver-sql,driver-turso): a cross-field $field refusal stops naming the two columns it compared (#7929, #7988) #8198 argument transfers and it does not: index.ts is untouched and attemptWithoutPoisoning is a protected class member, so there is no new public export, and the behaviour change is "a failure stops happening" rather than information removed from every caller.

    ⚠️ My dispatch assumption was directionally right and wrong on mechanism

    I told the dev that :4353 was the same bug class because "the fallback SELECT would also hit 25P02", flagged as an unverified inference. Measured on PG16: .forUpdate() on a missing row does not throw at all — it returns zero rows — so that catch was unreachable for the reason its comment gave. It is reachable for lock-level failures (40P01 deadlock, 55P03/57014 timeouts), and there the fallback read was converting a real error into an uninformative 25P02. Same conclusion (the site needed fixing), different and better-evidenced reason. The dev corrected the false comment in place rather than leaving it next to code it had just disproved — right call.

    ⚠️ The card's blast-radius framing is refuted, and this is the part worth carrying forward

    This card states "Single-tenant concurrency is fine at every level I tried" and scopes the defect to multi-org deployments. Measured, 5 rounds per level before the fix: 1 tenant × 2 → 0/5 failed, × 4 → 1/5, × 6 → 3/5, × 12 → 2/5, all 25P02. The tenant boundary is an amplifier, not a precondition — two cold counter rows make the window near-certain, but a single-org deployment inserting concurrently into a cold autonumber object was exposed too, just flakily. Single-org deployments on Postgres were hitting this as an intermittent first-insert failure.

    ⛔ I am not touching priority:* — that is triage's field — but flagging it explicitly: the p1 grade was set on a multi-org-only reading that measurement has now widened. The regression guard is still written cross-tenant, correctly, because that shape fails deterministically.

    Accepted deviation

    I instructed the dev to file a card for the reserved-number gap on a failed attempt. It recorded it in the report instead, reasoning that #8269's own body already describes it, that the dispatch scoped it out, and that after this fix the cold-race path stops producing failed attempts so the reported symptom disappears in practice. That reasoning is sound and a duplicate card would have been noise — accepted. The underlying behaviour (counters commit in their own transaction and never roll back with the caller's insert) is unchanged and remains ordinary sequence semantics unless someone rules otherwise.

    Next: CI is still converging (23 checks, several in_progress at time of writing). ⛔ No ready-flip on an in_progress read. Once every gate job reads completed: success → ready → merge queue → follow to MERGED. Two reds the dev reported locally are expected to be absent in CI: check:objectui-pin-fresh is ambient (it path-matches via .changeset/ and fires for any PR carrying one — confirmed ambient on #8198 earlier today), and check-dev-prereqs.mjs was an artifact of a deliberately scoped local build.


    Generated by Claude Code

  9. os-zhuang commented on Aug 13, 2026

    @os-zhuang
    Contributor

    PR #8279 → ready, queued. Head 92cd69a, all 25 check runs completed + success/skipped (job-level conclusions), path surface clean (.changeset/ + two driver-sql source files), mergeable_state: clean, Fixes #8269 intact.

    Temporal Conformance (live PG + MySQL) went green at 02:22:58Z — that is the job the new Postgres cell rides on, so the regression guard actually executed against a live PG rather than being skipped into a false pass. The two reds the dev reported from its local run (check:objectui-pin-fresh, check-dev-prereqs.mjs) have no corresponding CI job here, confirming both were local artifacts as it said.

    ⚠️ For triage, before this card closes on merge

    The card's stated scope — "Single-tenant concurrency is fine at every level I tried", hence multi-org-specific — is refuted by measurement. Single tenant, cold, concurrent, 5 rounds per level before the fix: ×2 → 0/5 failed, ×4 → 1/5, ×6 → 3/5, ×12 → 2/5, every failure the same 25P02. The tenant boundary makes the window near-certain but is not a precondition, so single-org Postgres deployments were also hitting this, as an intermittent first-insert failure with a permanent gap in the numbering afterwards.

    ⛔ I have not touched priority:* — that is triage's field. Flagging it because the p1 grade was assigned against the narrower reading, and the fix has now landed regardless; the point is the record, not the queue position.


    Generated by Claude Code

  10. baozhoutao commented on Aug 13, 2026

    @baozhoutao
    ContributorAuthor

    The everyday version of the same mechanism is recorded separately in the finding on burned numbers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingpm:queueReady for the PM dispatch looptarget:v17Release blocker for v17 — on the board until fixed, dropped as no longer valid, or accepted for GA

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions