Skip to content

Commit 7d1ff75

Browse files
claude[bot]os-devclaude
authored
fix(driver-sql): re-seed a stale autonumber counter instead of burning a number per failed create (#5495) (#6932)
* fix(driver-sql): re-seed a stale autonumber counter instead of burning a number per failed create (#5495) The counter bootstraps from the data-table MAX exactly once, in `getNextSequenceValue`'s `if (!existing)` branch; afterwards the data table is never consulted again. Any row landing by a path that bypasses `fillAutoNumberFields` — an `isSystem` seed replay, a `preserveAudit` import, or direct SQL — never raises the sequence, so once it sits below MAX it is permanently behind and every create collides, burns a number and fails the request until it has ground past the seeded range one 409 at a time. Measured on main @ 86e6f6c with the counter seeded at 10 and rows 11-39 landed by a bypass path: 29 caller-visible 409s before a create succeeded at CASE-00040 on attempt 30. Now: CASE-00040 on the caller's first attempt. `create()` re-seeds from MAX and retries (bounded) only when it can prove the collision was that counter's. The proof cannot be the conflicting column: `uniqueViolationColumn()` refuses composites, and ADR-0120 D3 makes the tenanted index an expression composite on which SQLite names only the index — so on this path the column is never determinable. All three of that export's states are handled explicitly; the indeterminate one is decided from the data (does the generated value already exist in this tenant partition?), never by a hand-written dialect word-list. Retry is confined to the no-caller-transaction case: inside a caller's transaction the sequence UPDATE rolls back with the INSERT, so nothing is burned, and on Postgres the transaction is aborted anyway. The "gaps are tolerated by design" docstring is reconciled with the change rather than left to contradict it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015XLbsWE5G58ybd1Leq6bNg * test(driver-sql): pin the three-state collision routing on Postgres and MySQL shapes (#5495) The SQLite tests cover the path end to end, but SQLite is the one dialect where the decision is easy. Postgres names a column in its DETAIL line (state 1/2) or a composite (state 3); MySQL names only an index and so can only ever reach state 3 — which means on MySQL the data probe is not a fallback, it is the whole mechanism. This package's unit suite boots SQLite only, so the shapes are injected rather than driven through live servers — same method and same reason as sql-driver-unique-violation-predicate.test.ts. The state-2 case asserts the part that matters most: a named column that is not ours ends the matter WITHOUT probing, even when the probe would have said yes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015XLbsWE5G58ybd1Leq6bNg --------- Co-authored-by: os-dev <steve@objectstack.ai> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 68feaad commit 7d1ff75

5 files changed

Lines changed: 815 additions & 14 deletions

File tree

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
fix(driver-sql): re-seed a stale autonumber counter instead of burning a number per failed create (#5495)
6+
7+
`getNextSequenceValue` bootstraps a counter from the data-table `MAX` exactly
8+
once, in its `if (!existing)` branch; after that the data table is never
9+
consulted again. Any row landing by a path that bypasses `fillAutoNumberFields`
10+
— an `isSystem` seed replay, a `preserveAudit` historical import (both
11+
strip-exempt under #5503 and keeping their explicit numbers), or direct SQL —
12+
therefore never raises the sequence, and once the counter sits below `MAX` it is
13+
permanently behind. Every subsequent create collided, burned a number and failed
14+
the request, until the counter had ground past the seeded range one 409 at a
15+
time. That is the "one-time storm per database" the filing reported from
16+
HotCRM's 17.0 GA sweep: 25 consecutive `409 UNIQUE_VIOLATION`s with the
17+
attempted number climbing by one per failure.
18+
19+
Measured on `main` @ `86e6f6c`, counter seeded at 10 with rows 11–39 landed by a
20+
bypass path: **29 caller-visible 409s before a create succeeded** at
21+
`CASE-00040` on attempt 30. After this change the same fixture serves
22+
`CASE-00040` on the caller's **first** attempt, and `last_value` reaches 40 by
23+
one re-seed rather than 29 burns.
24+
25+
`create()` now re-seeds the counter from the data-table `MAX` and retries
26+
(bounded, 3 attempts) — but only when it can *prove* the collision was that
27+
counter's.
28+
29+
**Why the proof is not the conflicting column.** The obvious predicate ("retry
30+
when the conflicting column is this autonumber field") needs
31+
`uniqueViolationColumn()` (#6544) to name a column, and on a tenanted autonumber
32+
it never does — for two independent reasons, both measured and both pinned by
33+
tests. The filing's own message is a composite
34+
(`UNIQUE constraint failed: crm_case.organization_id, crm_case.case_number`),
35+
which that export refuses by contract; and what this repo builds today is
36+
narrower still — ADR-0120 D3 makes the index
37+
`(COALESCE(organization_id,'__global__'), field)`, an *expression* index, on
38+
which SQLite reports `UNIQUE constraint failed: index 'uniq_…'` and names no
39+
column at all. The "column not determinable" limb is not an edge case on this
40+
path; it is the only limb that ever runs there.
41+
42+
All three of `uniqueViolationColumn()`'s states are handled explicitly, because
43+
collapsing any two of them silently is how a real 409 gets eaten:
44+
45+
1. a column is named and it is one this driver generated → re-seed and retry;
46+
2. a column is named and it is not → the duplicate is on a value the **caller**
47+
supplied, so the original error is rethrown untouched;
48+
3. no column is determinable → decided from the **data**, not the message: if
49+
the value this driver just generated is already present in the same tenant
50+
partition the counter covers, the collision was the counter's. If it is not,
51+
the error is rethrown. One indexed lookup, on the failure path only — the
52+
happy path is unchanged.
53+
54+
No fifth dialect word-list: the judgement is `isUniqueViolationError` +
55+
`uniqueViolationColumn` from `@objectstack/types`, per Prime Directive #12 and
56+
the #5841 precedent. The re-seed's `MAX` scan is deliberately not wrapped in a
57+
`catch`, so a read failure propagates instead of being folded into `0` or a
58+
stale value (#6114's rule, #5979's family).
59+
60+
Retrying is confined to the no-caller-transaction case. Inside a caller's
61+
transaction the sequence `UPDATE` shares that transaction and rolls back with
62+
the refused `INSERT`, so no number is burned (measured), and on Postgres a
63+
constraint failure aborts the transaction outright — the caller owns that retry.
64+
65+
The `getNextSequenceValue` docstring is reconciled rather than left to
66+
contradict the code: a rolled-back insert burning a number is still by design,
67+
and that sentence used to read as though it also covered a *persistently
68+
failing* insert, which was the defect.
69+
70+
Inherited by `TursoDriver` (local/replica) and `SqliteWasmDriver`, each pinned
71+
by its own test rather than assumed from the base class (#6203). Turso's
72+
**remote** transport is unaffected in both directions: it overrides `create` and
73+
never enters `fillAutoNumberFields`, so it has neither the defect nor the fix.

0 commit comments

Comments
 (0)