Skip to content

Commit da1126a

Browse files
os-warrenclaude
andauthored
fix(metadata-protocol): write the merged autonumber high-water mark before retiring the __global__ counter (#12554)
* fix(metadata-protocol): write the merged autonumber high-water mark before retiring the __global__ counter The #8686 seed/API tenancy handoff ran an UPDATE of the organization-scoped _objectstack_sequences row followed by an unconditional DELETE of the '__global__' one. On a fresh install there is no organization-scoped row yet, so the UPDATE matched nothing (a success on every dialect), the DELETE ran anyway, and the counter table was left empty — sending SqlDriver.getNextSequenceValue back into its one-time MAX(data) bootstrap and re-issuing an already-allocated business identifier. The handoff is now one ordered decision per scope: write the merged mark (INSERT when the destination row is absent, UPDATE when it is not), read it back, and only then retire the '__global__' row by its own stored key_hash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o * test(metadata-protocol,cli): seed the sequences fixtures with the key the platform actually stores CI caught the #12394 handoff writing a SECOND counter row for one logical sequence, on live MySQL and on SQLite alike. Root cause is the fixtures, not the repair: both hand-seeded `key_hash` as an invented string (`'h1'`/`'h2'` and `'h_global'`/`'h_org'`), which was inert for as long as the repair addressed counter rows by `(object, field, tenant_id)`. #12394 addresses the destination row by `key_hash` — the table's only key — so an invented hash describes a table no install can hold: the org row reads ABSENT and a second row is inserted beside it. Measured: the driver stores `key_hash = sha256(object US tenant US field US scope)` for every row it writes, and `ensureSequencesKeyHashShape` recomputes the same hash for every legacy row it migrates. - cli: take the hash from the driver's own `sequenceKeyHash`, so the fixture is the same bytes the only production writer would have written and cannot drift. - metadata-protocol live-MySQL: spell the derivation independently (this package does not depend on driver-sql), making it a third spelling and therefore a pin on it; give `key_hash` its real PRIMARY KEY. - metadata-protocol unit: new #12394 suite over a KEYED store that answers the probe by its parameter and enforces the primary key. The INSERT-vs-UPDATE decision had no unit coverage keyed by a real hash — every existing fake matched on statement shape and handed back its one row for any key. `sequenceKeyHash` is exported from the module for that suite; it is NOT re-exported from the package index, so the published surface is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3a04b01 commit da1126a

7 files changed

Lines changed: 815 additions & 50 deletions
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
fix(metadata-protocol): the #8686 tenancy backfill writes the merged autonumber high-water mark before it deletes anything (#12394)
6+
7+
The seed/API tenancy handoff destroyed the counter it was supposed to move. It ran two
8+
independent statements — an `UPDATE` of the organization-scoped `_objectstack_sequences`
9+
row, then an unconditional `DELETE` of the `'__global__'` one — and on a **fresh install**
10+
there is no organization-scoped row yet, because no API create has happened. The `UPDATE`
11+
matched nothing, which is a success on every dialect; the `DELETE` ran regardless; the
12+
counter table was left empty. `SqlDriver.getNextSequenceValue` then re-entered the
13+
`if (!existing)` bootstrap its own docstring reserves for first allocation, re-derived the
14+
counter from `MAX(data)`, and **re-issued a business identifier that had already been
15+
handed out** — measured on 17.1.0: `ACC-000009` on two different records.
16+
17+
The zero-row case is the *normal* first-boot shape, not an edge case: it is precisely the
18+
shape `buildSplitProbeSql`'s `LEFT JOIN` was widened to catch, so the repair fired on
19+
exactly the installs where its merge loop body never executed.
20+
21+
The handoff is now one ordered decision per scope:
22+
23+
1. **write** the merged mark — `INSERT` when the organization-scoped row is absent,
24+
`UPDATE` when it exists;
25+
2. **read it back** — "the statement did not throw" was never evidence a row was written,
26+
and an `UPDATE` matching zero rows is exactly the defect above;
27+
3. **then** retire the `'__global__'` row, addressed by its own stored `key_hash`, so a
28+
retirement can only ever hit the row whose mark was just merged.
29+
30+
A throw at any step leaves the `'__global__'` row in place — which is the state the next
31+
boot's split probe detects and retries — so a failed repair now loses nothing.
32+
33+
Per **scope**, because a `{YYYYMMDD}` / `{field}` / per-parent format runs one counter row
34+
per rendered prefix. The old merge was scope-blind in both directions: it could raise every
35+
scope's counter to one merged value, and it deleted every scope's `'__global__'` row.
36+
37+
The merge rule itself is unchanged and is the 2026-08-15 ruling's: the greater of the two
38+
**counters**, never the data max. That rule is the whole point — a counter is allowed to
39+
sit ahead of its rows (a rolled-back insert burns a number, by design), and that gap is
40+
exactly what the old handoff threw away.
41+
42+
Graded **patch**: a defect repair inside an existing migration. It adds no export to
43+
`@objectstack/metadata-protocol`'s public index — the new SQL builders are module-scoped
44+
for their own unit tests, matching the index's own recorded rule that an export added so a
45+
test can import a value is the shape to catch before it ships.
46+
47+
No change to the allocator. Reaching `if (!existing)` is not evidence of lost state — a new
48+
tenant, a new day and a new `{field}` group each reach it legitimately, and a destroyed
49+
counter leaves no row behind to tell the two apart — so a guard there would fire on the hot
50+
path and still not detect this. The repair belongs where the state was destroyed.

packages/cli/src/utils/platform-migrations-arming.integration.test.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,9 +149,24 @@ async function writeDamagedInstall(): Promise<void> {
149149
t.bigInteger('last_value').notNullable().defaultTo(0);
150150
t.timestamp('updated_at');
151151
});
152+
// [#12394] `key_hash` is DERIVED, never invented. It used to be seeded here as
153+
// the placeholders `'h1'`/`'h2'`, which was harmless only for as long as the
154+
// repair addressed counter rows by `(object, field, tenant_id)`: nothing read
155+
// the column, so any string did. #12394's handoff addresses the destination
156+
// row by `key_hash` — the key the table is actually keyed on — so a fixture
157+
// carrying an invented hash describes a table the platform cannot produce, and
158+
// the repair correctly reads the org row as ABSENT and inserts a SECOND one.
159+
//
160+
// Taking the hash from the driver's own `sequenceKeyHash` rather than
161+
// re-spelling it here is the point: this fixture is now the same bytes the
162+
// only production writer of this table would have written, and it cannot drift
163+
// from it. `ensureSequencesKeyHashShape` recomputes the same hash for every
164+
// legacy row it migrates, so this is the shape of every real install.
165+
const keyHash = (object: string, tenantId: string, field: string, scope = ''): string =>
166+
(seed as any).sequenceKeyHash(object, tenantId, field, scope);
152167
await k(SEQUENCES_TABLE).insert([
153-
{ key_hash: 'h1', object: 'crm_case', tenant_id: GLOBAL_TENANT, field: 'case_number', scope: '', last_value: SEEDED_LAST_VALUE },
154-
{ key_hash: 'h2', object: 'crm_case', tenant_id: ORG_ID, field: 'case_number', scope: '', last_value: 1 },
168+
{ key_hash: keyHash('crm_case', GLOBAL_TENANT, 'case_number'), object: 'crm_case', tenant_id: GLOBAL_TENANT, field: 'case_number', scope: '', last_value: SEEDED_LAST_VALUE },
169+
{ key_hash: keyHash('crm_case', ORG_ID, 'case_number'), object: 'crm_case', tenant_id: ORG_ID, field: 'case_number', scope: '', last_value: 1 },
155170
]);
156171
await seed.disconnect();
157172
}

packages/metadata-protocol/src/migrations/seed-tenancy-backfill.live-mysql.test.ts

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
*/
4747

4848
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
49+
import { createHash } from 'node:crypto';
4950
import mysql from 'mysql2/promise';
5051
import {
5152
backfillSeedTenancy,
@@ -68,6 +69,32 @@ const DB = currentLiveMysqlDatabase();
6869
const OBJECT = 'os9381_case';
6970
const FIELD = 'case_number';
7071

72+
/**
73+
* The row key of `_objectstack_sequences`, spelled the way its only production
74+
* writer spells it (#12394).
75+
*
76+
* This fixture used to seed `key_hash` as the placeholders `'h_global'` and
77+
* `'h_org'`. That was invisible for as long as the repair addressed counter rows
78+
* by `(object, field, tenant_id)` — nothing read the column, so any string did.
79+
* #12394's handoff addresses the destination row by `key_hash`, which is the key
80+
* the table is actually keyed on, so an invented hash describes a table no
81+
* install can hold: the repair reads the organization row as ABSENT and inserts
82+
* a SECOND counter for one logical sequence.
83+
*
84+
* Spelled here rather than imported because `metadata-protocol` does not depend
85+
* on `driver-sql` — which makes this a THIRD independent spelling of the same
86+
* derivation, and therefore a pin on it: a separator or field-order change in
87+
* `seed-tenancy-backfill.ts` stops matching these rows and this suite goes red.
88+
* The separator is the ASCII unit separator, written as the escape \u001f and
89+
* never as a raw control byte — the same discipline the module and the driver
90+
* both keep.
91+
*/
92+
function sequenceKeyHash(object: string, tenantId: string, field: string, scope: string): string {
93+
return createHash('sha256')
94+
.update(`${object}\u001f${tenantId}\u001f${field}\u001f${scope}`)
95+
.digest('hex');
96+
}
97+
7198
if (!MYSQL_URL && EXPECT_LIVE) {
7299
describe('#9381 live MySQL', () => {
73100
it('OS_TEST_MYSQL_URL must be set — this runner declared it provisioned a server', () => {
@@ -113,9 +140,18 @@ describe.skipIf(!MYSQL_URL)('#9381 seed-tenancy backfill on a LIVE MySQL', () =>
113140
// Column names spelled the way the driver's own `createSequencesTable`
114141
// spells them; `last_value` is quoted here for the same reason the migration
115142
// has to quote it (see the reserved-word assertion below).
143+
//
144+
// [#12394] `key_hash` carries its real PRIMARY KEY. The driver declares it
145+
// `.notNullable().primary()`, and it is the ONLY key this table has — no
146+
// unique index stands behind `(object, tenant_id, field, scope)`. Seeding it
147+
// as a plain column let a repair that wrote a SECOND row for one logical
148+
// counter land quietly as an extra row instead of an `ER_DUP_ENTRY`; with
149+
// the real key here, that defect can only ever be an error on the two
150+
// dialects that enforce it.
116151
await conn.query(
117152
`CREATE TABLE \`${SEQUENCES_TABLE}\` (` +
118-
'`key_hash` VARCHAR(64), `object` VARCHAR(64), `tenant_id` VARCHAR(64), ' +
153+
'`key_hash` VARCHAR(64) NOT NULL PRIMARY KEY, `object` VARCHAR(64), ' +
154+
'`tenant_id` VARCHAR(64), ' +
119155
'`field` VARCHAR(64), `scope` VARCHAR(255) NOT NULL DEFAULT \'\', ' +
120156
'`last_value` INT, `updated_at` DATETIME(3))',
121157
);
@@ -127,8 +163,12 @@ describe.skipIf(!MYSQL_URL)('#9381 seed-tenancy backfill on a LIVE MySQL', () =>
127163
await conn.query("INSERT INTO `sys_organization` (`id`) VALUES ('org_live')");
128164
await conn.query(
129165
`INSERT INTO \`${SEQUENCES_TABLE}\` (\`key_hash\`, \`object\`, \`tenant_id\`, \`field\`, \`last_value\`) ` +
130-
`VALUES ('h_global', '${OBJECT}', '${GLOBAL_TENANT}', '${FIELD}', 38), ` +
131-
`('h_org', '${OBJECT}', 'org_live', '${FIELD}', 4)`,
166+
`VALUES (?, '${OBJECT}', '${GLOBAL_TENANT}', '${FIELD}', 38), ` +
167+
`(?, '${OBJECT}', 'org_live', '${FIELD}', 4)`,
168+
[
169+
sequenceKeyHash(OBJECT, GLOBAL_TENANT, FIELD, ''),
170+
sequenceKeyHash(OBJECT, 'org_live', FIELD, ''),
171+
],
132172
);
133173
// The card's own repro: seeded rows carry NULL, API rows carry the org, and
134174
// CASE-00001/2 were minted on BOTH sides.

packages/metadata-protocol/src/migrations/seed-tenancy-backfill.null-seam.test.ts

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -250,9 +250,17 @@ describe('#10789 a seam that answers with no rows still reports no-split', () =>
250250
// installs it exists for. This seam answers every SELECT and hands back a
251251
// NON-result-set for every write — and the repair must still complete.
252252
const writes: string[] = [];
253-
const exec: SeedTenancyExec = async (sql: string) => {
254-
if (sql.startsWith('UPDATE') || sql.startsWith('DELETE')) {
253+
// #12394: the handoff reads its own write back before retiring anything, so
254+
// the counter row is modelled rather than assumed — the WRITES still answer
255+
// with a non-result-set, which is what this case exists to pin.
256+
let orgCounter: Record<string, unknown> | undefined;
257+
const exec: SeedTenancyExec = async (sql: string, params?: unknown[]) => {
258+
if (sql.startsWith('UPDATE') || sql.startsWith('DELETE') || sql.startsWith('INSERT')) {
255259
writes.push(sql.slice(0, 6));
260+
if (sql.startsWith('INSERT')) orgCounter = { last_value: Number(params?.[5]) };
261+
if (sql.startsWith('UPDATE') && sql.includes('_objectstack_sequences')) {
262+
orgCounter = { last_value: Number(params?.[0]) };
263+
}
256264
return { affectedRows: 3 }; // not a result set, by design
257265
}
258266
if (sql.includes('WHERE 1 = 0')) return [];
@@ -263,7 +271,8 @@ describe('#10789 a seam that answers with no rows still reports no-split', () =>
263271
}
264272
if (sql.includes(ORGANIZATION_TABLE)) return [{ id: 'org_a' }];
265273
if (sql.includes('rows_holding')) return [];
266-
if (sql.includes('tenant_id')) return [{ tenant_id: 'org_a', last_value: 1 }];
274+
if (sql.includes('"key_hash" = ?')) return orgCounter ? [orgCounter] : [];
275+
if (sql.includes('tenant_id')) return [{ key_hash: 'hash-global', scope: '', last_value: 38 }];
267276
return [];
268277
};
269278

@@ -274,6 +283,9 @@ describe('#10789 a seam that answers with no rows still reports no-split', () =>
274283
expect(result.organizationId).toBe('org_a');
275284
expect(writes).toContain('UPDATE');
276285
expect(writes).toContain('DELETE');
286+
// The write that used to be missing: on a fresh install there is no
287+
// organization-scoped counter row to raise, so the mark is INSERTed.
288+
expect(writes).toContain('INSERT');
277289
});
278290
});
279291

0 commit comments

Comments
 (0)