Skip to content

fix(metadata-protocol): write the merged autonumber high-water mark before retiring the __global__ counter - #12554

Merged
os-warren merged 3 commits into
mainfrom
claude/issue-12394-autonumber-highwater
Aug 26, 2026
Merged

fix(metadata-protocol): write the merged autonumber high-water mark before retiring the __global__ counter#12554
os-warren merged 3 commits into
mainfrom
claude/issue-12394-autonumber-highwater

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes#12394

The defect, reproduced before it was repaired

The #8686 seed/API tenancy handoff destroyed the counter it existed to move. It ran two
independent statements — buildCounterMergeSql, an UPDATE of the organization-scoped
_objectstack_sequences row, then buildGlobalCounterDeleteSql, an unconditional
DELETE of the '__global__' one. On a fresh install there is no organization-scoped row
yet, because no API create has happened: the UPDATE matched nothing (a success on every
dialect), the DELETE ran regardless, and the counter table was left empty.

That is the normal first-boot shape, not an edge case — it is precisely the shape
buildSplitProbeSql's LEFT JOIN was widened to catch, so the repair fired on exactly the
installs where its merge loop body never executed once.

Measured here on origin/main, better-sqlite3, seed + burn + sign-up + create, with the
built artifact proved to carry the un-fixed module (ablation-dist-preflight, marker
absent from all 24 built files):

{
"seed": { "seededMax": "ACC-000009", "counters": ["__global__=9"] },
"afterBurn": { "rows": 8, "dataMax": "ACC-000008", "counters": ["__global__=9"] },
"afterHandoff": { "status": "applied", "counters": [] },
"firstApiCreate": "ACC-000009",
"reIssuedAnAllocatedNumber": true
}

ACC-000009 handed out a second time, to a different record — the card's measurement,
reproduced. The same harness on this branch:

{
"afterHandoff": { "status": "applied", "counters": ["org_mssymr19xzd645gv=9"] },
"firstApiCreate": "ACC-000010",
"reIssuedAnAllocatedNumber": false
}

The burn is what makes the two distinguishable. With no number burned, the driver's
MAX(data) rescan lands on exactly the value the counter held, and both trees mint
ACC-000010.

The fix

mergeSplitCounter replaces the two hopeful statements with one ordered decision, per
scope
:

  1. Write the merged mark — INSERT when the organization-scoped row is absent,
    UPDATE when it exists. The absent case is first boot.
  2. Read it back. "The statement did not throw" was never evidence a row was written,
    and an UPDATE matching zero rows is exactly this defect. The destination row is
    re-read and must hold at least the merged value.
  3. Then delete — the '__global__' row, addressed by its own stored key_hash, so
    a retirement can only ever hit the row whose mark was just merged.

A throw at any step leaves the '__global__' row in place, which is the state the next
boot's split probe detects and retries; a failed repair now loses nothing. Partial progress
across scopes is safe for the same reason.

Per scope, because a {YYYYMMDD} / {field} / per-parent format runs one counter row per
rendered prefix. The old merge was scope-blind in both directions: it could raise every
scope's counter to one merged value, and it deleted every scope's '__global__' row.

The merge rule itself is unchanged and is the 2026-08-15 ruling's: the greater of the two
counters, never the data max.

The key_hash objection, and why it is discharged rather than dodged

The deleted comment argued the delete was the design: the driver keys counters by a
key_hash computed in app code, "and a hash spelled two ways is a counter the driver
cannot find". That is right about the hazard and wrong about the remedy — deleting the row
hands the merged mark to nobody, and the driver's one-time MAX(data) bootstrap then
re-derives a number it has already issued, which its own docstring forbids ("after the
one-time bootstrap the data table is never consulted again").

So the spelling is duplicated deliberately — the same way GLOBAL_TENANT and the seed
loader's platform-namespace regex already are here, because metadata-protocol must not
depend on a driver — and it is controlled by a test that can only pass if the two agree:
the new {field}-scoped pin drives a real SqlDriver across a burned number, so a hash
differing by one byte leaves a row the driver never reads, the driver re-enters its
bootstrap, and the burned number comes back. Divergence is a red test, not a silent counter.

Table shape is asked of the database, once, in the WHERE 1 = 0 idiom this module already
uses: a table without key_hash refuses the probe, and that refusal is the answer. Those
installs are keyed by (object, tenant_id, field) — which is what SqlDriver itself falls
back to on them, not a dialect invented here.

The second signal the card raised — declined, with reasons

The card asks whether driver-sql re-entering its MAX(data) bootstrap after the counter
table has been initialised deserves its own guard. Declined, deliberately, and the
reasoning is recorded in the module docstring rather than left in this PR:

  • Reaching if (!existing) is not evidence of lost state. That branch is the normal
    path for every new tenant, every new day, and every new {field} group — on a mature
    install, constantly. A guard keyed on "the table is not empty" would fire on the hot
    allocation path.
  • A destroyed counter leaves no row behind, so nothing in the schema distinguishes
    "this key never existed" from "this key existed and was deleted". The guard could not
    detect this defect even where it fired.
  • The module's own standing rule is that this family is not a counter bug and must never be
    repaired by making the allocator smarter. The state was destroyed upstream; that is where
    it is repaired.

If the maintainer wants a signal there anyway, it wants a tombstone the backfill writes —
a design decision, not a hunk to append here.

Public surface

No clause ② widening.packages/metadata-protocol/src/index.ts is untouched
(git diff --stat origin/main...HEAD -- packages/metadata-protocol/src/index.ts is empty).
The new SQL builders are module-scoped so the module's own unit tests can import them —
matching the index's own recorded rule that an export added so a cross-package test can
import a value is the shape to catch before it ships. needs:contract-review can come off
on that basis. One module-private helper, buildOrgCounterProbeSql, is deleted; it had no
consumer anywhere in the tree (zero-hit scan run with buildCounterMergeSql as the positive
control, which returned 4 files).

Verification

Ablation, one leg, prediction written before the mutation ran (direction and exact
count), mutation proved on disk with anchored grep -cF counts before any result was read,
restored under trap ... EXIT INT TERM and the restore verified by an empty git status:

  • Predicted: RED, 4 failed / 7 passed of 11, naming all four cases and the seven that
    stay green.
  • Observed: Tests 4 failed | 7 passed (11) — the same four, no others.
  • On disk before reading anything: mergeSplitCounter 3 to 0, buildCounterInsertSql 2 to
    0, and the two-way anchor buildOrgCounterProbeSql 0 to 2.
  • Rebuilt each leg and proved reach: ablation-dist-preflight @objectstack/metadata-protocol mergeSplitCounter --absent
    on the mutated leg, present on the restored one. The package resolves through its
    exports to dist/, so an unbuilt mutation reads green.

Two of those four failures are pre-existing assertions this PR changes on purpose:
[ruling: option 1] and [wiring] asserted readSequences(driver) was [] after the
handoff. An empty counter table is the symptom, so the suite was pinning the defect as the
contract. Both now assert the mark survives in the organization's own row.

Green on f7b251cbae, the final commit:

checkresult
pnpm lint (eslint . --no-inline-config, whole repo)lint-rc=0
pnpm --filter @objectstack/metadata-protocol exec vitest run src/migrations/Test Files 9 passed / 2 skipped, Tests 191 passed / 10 skipped
pnpm --filter @objectstack/runtime exec vitest run (both tenancy-split integration files)Test Files 2 passed (2), Tests 15 passed (15)
pnpm --filter @objectstack/runtime typecheck (tsc --noEmit)rc 0
pnpm check:type-check-debt (--re-measure, over a full workspace build)OK — 32 ledger entr(ies) re-measured, 1843 raw tsc error(s) total, none above its recorded number

Gate union derived, not recalled — node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack
over the real changeset (4 paths, three-dot from the merge base). All 25 matched families
run locally, each exit code captured before any pipe, all rc=0: nul-bytes ·
cross-package-test-inputs (both spellings) · durability-log-level ·
objectql-double-limit · page-declaration-shape · published-files · slot-lookup ·
test-source-alias · type-source-resolution · ci-filter-parity ·
comment-mask-adoption · plugin-teardown-shape · docs-audit/check-affected-docs ·
docs-audit/check-drift-comment · query-options-erasure · engine-double-contract ·
where-matcher · type-check-coverage · type-check-debt · changeset-gate-self-tests ·
objectui-changeset · check-adr-0087-registration · check-changeset-no-major ·
check-empty-changeset · release-rehearsal-clone --self-test.

Changeset

patch on @objectstack/metadata-protocol, graded deliberately: a defect repair inside an
existing migration, no export added to the package index, no API shape changed. The one
argument for minor would be new published surface, and there is none. @objectstack/runtime
takes no bump — its only change is test code.

Serial constraint

#12395's band (the organizationIds.length !== 1 guard and its warning payload, lines
1016–1025 on origin/main) is untouched. The earliest line this diff changes in that
function is 1083.


Generated by Claude Code

…efore 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
@github-actions

github-actionsBot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/metadata-protocol, touching 14 documentable anchor(s).

12 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/wire-format.mdx(via updated_at (literal))
  • content/docs/automation/webhooks.mdx(via updated_at (literal))
  • content/docs/deployment/seed-tenancy-repair.mdx(via last_value (literal), tenant_id (literal), updated_at (literal))
  • content/docs/permissions/record-view-auditing.mdx(via tenant_id (literal))
  • content/docs/permissions/system-context.mdx(via updated_at (literal))
  • content/docs/protocol/kernel/config-resolution.mdx(via tenant_id (literal))
  • content/docs/protocol/kernel/http-protocol.mdx(via updated_at (literal))
  • content/docs/protocol/kernel/realtime-protocol.mdx(via updated_at (literal))
  • content/docs/protocol/objectql/schema.mdx(via updated_at (literal))
  • content/docs/protocol/objectql/security.mdx(via updated_at (literal))
  • content/docs/protocol/objectql/state-machine.mdx(via updated_at (literal))
  • content/docs/ui/views.mdx(via updated_at (literal))

4 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/implementation-status.mdx(via tenant_id (literal))
  • content/docs/releases/v15.mdx(via updated_at (literal))
  • content/docs/releases/v16.mdx(via updated_at (literal))
  • content/docs/releases/v17.mdx(via tenant_id (literal), updated_at (literal))

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.

What this run could not see
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 7 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json b307bfd2aef1067317900f73dafe15478ad8fe22packageMentionDocs.

Which tree this was computed on

This run read content/docs from 1f71a49193c7b4de17cffd7daa138c07b649bc7b — the merge of head 7ec7d8d4b43e9d6e34b7fa8331876317f994ea46 into base b307bfd2aef1067317900f73dafe15478ad8fe22, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 1f71a49193c7b4de17cffd7daa138c07b649bc7b && git checkout 1f71a49193c7b4de17cffd7daa138c07b649bc7b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin b307bfd2aef1067317900f73dafe15478ad8fe22 7ec7d8d4b43e9d6e34b7fa8331876317f994ea46 && git checkout -B drift-repro b307bfd2aef1067317900f73dafe15478ad8fe22 && git merge --no-ff 7ec7d8d4b43e9d6e34b7fa8331876317f994ea46
node scripts/docs-audit/affected-docs.mjs --json b307bfd2aef1067317900f73dafe15478ad8fe22

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs b307bfd2aef1067317900f73dafe15478ad8fe22 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 26, 2026
@os-warren
os-warren marked this pull request as ready for review August 26, 2026 10:26
@os-warren
os-warren enabled auto-merge August 26, 2026 10:26
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — accepted, flipped ready, auto-merge armed

I re-measured the three claims a PM should never take on the implementer's word, on origin/main... the PR head, independently of the report:

claimhow I checked itresult
no clause ② wideninggit diff --stat origin/main...HEAD -- packages/metadata-protocol/src/index.tsempty — holds. needs:contract-review removed from #12394
#12395's reserved band 1016–1025 untouchedgit diff -U0 hunk headers on seed-tenancy-backfill.tsold-side hunks at 107, 109, 660, 662, 664, 668, 1083, 1091, 1122nothing in 1016–1025, earliest in-function line is 1083 exactly as stated
scope not widenedgit diff --name-only5 files, all named in the card. No drive-by

The part that earned the accept

Two of the four ablation reds were pre-existing assertions this PR reversesreadSequences(driver) was pinned as [] after the handoff. That is the single riskiest edit shape in a defect PR, because it is indistinguishable at a glance from quietly editing a red pin green. Here it is the opposite, and it is handled the way it has to be: the reversal is declared in the report, declared in the PR body, and declared in the test file itself, on the line —

"This assertion used to read toEqual([]), which pinned the defect as if it were the contract: an empty counter table is exactly what sends the driver back into its one-time MAX(data) bootstrap."

The suite was pinning the symptom as the contract. Reversing it is the fix, and a reader six months from now will find out why without leaving the file.

The card's explicit trap is cleared too: it warned that a "no duplicates after seed + sign-up + create" pin is green with and without the defect, so any pin must burn a number first. Both new pins delete the top-numbered row before exercising the handoff. Read the diff to confirm rather than the prose.

And the {field}-scope pin is doing more work than the card asked for. The fix duplicates the driver's key_hash spelling into metadata-protocol (correctly — the package must not depend on a driver), which is a cross-package agreement with nothing structural holding it. That pin constructs a real SqlDriver and drives it across a burned number, so a hash differing by one byte leaves a row the driver never reads, the driver re-enters its bootstrap, and the burned number comes back — a red test, not a silent counter. Verified the new SqlDriver({…}) is actually there and not a double.

Honest non-measurements, recorded rather than smoothed over

Both are declared in the report and neither is being counted as a pass:

Open question carried to the maintainer, not answered here

The card's third item — a driver-sql guard against re-entering the MAX(data) bootstrap after the counter table has been initialised — is declined in this PR, with the reasoning in the module docstring: if (!existing) is the normal path for every new tenant, day and {field} group, so a guard there fires on the hot allocation path; and a destroyed counter leaves no row behind, so nothing distinguishes "never existed" from "was deleted" — it could not detect this defect even where it fired.

That reasoning is sound on its own terms and I am not overruling it. But the alternative the implementer names — a tombstone the backfill writes, so "this counter was retired here" becomes a fact in the schema rather than an inference — is the only option that would really detect it, and it is a stored-shape design decision, not a hunk. That is the maintainer's call, not the seat's. Flagged here; if it is wanted it gets its own card rather than an append to this one.

CI is the remaining gate. Auto-merge armed; #12395 unblocks the moment this lands.


Generated by Claude Code

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

⚠️ CI is red — dispatched for repair, not waiting it out

Run 32957893593, three failures. Recording the diagnosis so far so the repair does not start from zero, and so nobody reads the armed auto-merge as "this is fine".

checkconclusionreading
Temporal Conformance (live PG + MySQL)failurevery likely ours — see below
Test Core (1/6)failure16-minute run; failing step not yet located
Test Core (rollup)failurederived, not independent

The rollup is not a third failure

"test-1-of-6 MISSING … 1 of 6 declared shard(s) published no positive attestation — a shard that never ran cannot be counted as passing (#6082)." It is shard 1's failure restated. Worth saying because three red checks read as three problems and it is two.

The one that points at this diff

The Postgres service log carries this twice:

ERROR: duplicate key value violates unique constraint "_objectstack_sequences_pkey"
DETAIL: Key (key_hash)=(b8c4320…6cca) already exists.
STATEMENT: insert into "_objectstack_sequences" ("field","key_hash","last_value","object","scope","tenant_id") values ($1,$2,$3,$4,$5,$6)

That INSERT is this PR's, and nothing else in the diff writes that table.

Working hypothesis, stated as a hypothesis because it is not yet confirmed from the failing assertion: the "is the org-scoped row absent?" probe and the table's primary key are not the same key. The probe asks by (object, field, tenant_id, scope); the PK is key_hash. A row that already exists under that hash but does not match the probe's predicate reads as absent, the INSERT fires, and the PK rejects it.

If that holds, it is the sharp edge this PR's own body identified and believed it had covered — the duplicated key_hash spelling, "controlled by a test that can only pass if the two agree". The {field}-scope pin does drive a real SqlDriver, and I verified that when I reviewed it. But it drives SQLite, and this is Postgres. The control was real and its coverage was narrower than the claim it was supporting. That is the honest read, and it is also exactly the gap the report flagged when it declared the live-MySQL suite not measured rather than passing — the declaration was correct and the risk it named landed.

⚠️ Caveat against over-reading that log: the same service log contains deliberate negative-path errors from other suites (os8269_poison_probe_pkey, os11161_no_such_relation), so a PG ERROR line is not by itself a failure. The failing assertion still has to be read from the job step output, and the dispatch says so.

What was NOT concluded

Test Core (1/6)'s own check-test-completeness step printed OK (6 of 6 scheduled package(s) reported … 3572 test(s) declared and all accounted for) — so the shard's tests accounted for themselves and the failure is in some other step, which I could not reach from the log tail. Not attributed to this diff yet, and not attributed away from it either.

Disposition

Dispatched to repair, with instructions to rule out "not this PR's" against main's own runs for both jobs before changing anything, and to add a pin that closes the dialect gap rather than one more SQLite case — a pin that only runs on SQLite is the same blind spot one layer along.

Auto-merge stays armed: it fires only on green, so a red head cannot slip through, and a validated fix merges without another round trip. ⛔ Nothing gets pushed to this branch that has not been validated first.

The serial hold on #12395 stands until this lands.


Generated by Claude Code

… 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
@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

CI red on f7b251cbae — root cause and fix (7ec7d8d4b4)

What was failing

Three checks in run 32957893593, but only two independent failures:

  1. Test Core (1/6)packages/clisrc/utils/platform-migrations-arming.integration.test.ts:268, on SQLite.
  2. Temporal Conformance (live PG + MySQL)packages/metadata-protocolsrc/migrations/seed-tenancy-backfill.live-mysql.test.ts:219, on live MySQL.
  3. Test Core (rollup) — derived from (1) (test-1-of-6 MISSING). Not chased separately.

Both failed the same way: after the repair, two organization-scoped counter rows instead of one.

+ "last_value": 1, (cli, org_x) + "last_value": 4, (mysql, org_live)
"last_value": 38, "last_value": 38,

Root cause — the fixtures, not the repair

Both fixtures hand-seeded key_hash as an invented string: 'h1'/'h2' in the CLI test, 'h_global'/'h_org' in the live-MySQL test. That was inert for as long as this migration addressed counter rows by (object, field, tenant_id) — nothing read the column, so any string did.

This PR addresses the destination row by key_hash, which is the key the table is actually keyed on. An invented hash therefore describes a table no install can hold: the probe correctly reports the organization row ABSENT, the new INSERT fires, and a second counter lands beside the first.

Measured rather than assumed — a real SqlDriver allocating one number:

rows written by the driver: [{"key_hash":"b0f1b781…8183","object":"probe_case",
"tenant_id":"org_x","field":"case_number","scope":"","last_value":1, …}]
stored = b0f1b7813761d6fa7c6e666edff2fafce6a0d64e7e3834ac1f0549a9b8728183
recomp = b0f1b7813761d6fa7c6e666edff2fafce6a0d64e7e3834ac1f0549a9b8728183
MATCH = true

key_hash is sha256(object ␟ tenant_id ␟ field ␟ scope) for every row the driver writes, and ensureSequencesKeyHashShape recomputes the same hash for every legacy row it migrates. It is also the table's only key — createSequencesTable declares key_hash.notNullable().primary() and no unique index stands behind (object, tenant_id, field, scope). So the invariant is total, and the production code in this PR is correct as written. No production behaviour changed in this commit.

Note this was never a dialect gap: it reproduces identically on SQLite and on MySQL. The PG duplicate key … _objectstack_sequences_pkey lines in the Postgres service log are not this PR — they are the deliberate concurrent-allocation race in sql-driver-autonumber-cold-race.test.ts (os8269_cold_race, last touched by ecb39ea22d), which is why they sit next to the equally deliberate os8269_poison_probe_pkey line.

The fix

  • packages/cli/…/platform-migrations-arming.integration.test.ts — the hash now comes from the driver's own sequenceKeyHash, so the fixture is the same bytes the only production writer would have written and cannot drift from it.
  • packages/metadata-protocol/…/seed-tenancy-backfill.live-mysql.test.ts — the derivation is spelled independently (this package does not depend on driver-sql), making it a third spelling and therefore a pin on it. key_hash also carries its real PRIMARY KEY now, so the fixture matches the shape it claims to exercise.
  • packages/metadata-protocol/…/seed-tenancy-backfill.test.ts — the new pin (below).

sequenceKeyHash is exported from the module for that suite. It is not re-exported from the package index, so the published surface is unchanged.

The new pin

The reason this cost a CI round-trip is that the cheap layer was blind. Every fake seam in the unit suite dispatches on statement shapesql.includes('"key_hash" = ?') — and hands back its one row without ever reading the parameter. That models a database where every key addresses the same row, so the one decision this handoff makes, INSERT-vs-UPDATE keyed by key_hash, cannot fail there. The UPDATE branch (an organization row that already exists) reached CI only through the two integration fixtures that were wrong.

Added #12394 the counter handoff writes the row the driver will read — a fake _objectstack_sequences that is a real keyed store: rows live in a Map under their own key_hash, the probe answers by the parameter it was given, and the INSERTrefuses a key already present, because that is what the primary key does. Three cases: first boot (INSERT), the CI regression (an existing row is RAISED, never duplicated), and never-lowered (an organization row ahead of __global__ keeps its mark).

Ablation, to show it can fail — sequenceKeyHash(object, organizationId, field, scope) at the destination probe mutated to append a marker (mutation confirmed on disk: anchor count 1 → 0, marker count 0 → 1, git diff --stat non-empty; restored by an EXIT/INT/TERM trap, marker count back to 0 and a clean tree):

ABLATED metadata-protocol unit: Tests 3 failed | 48 passed (51) ← exactly the 3 new cases
ABLATED live MySQL: Tests 2 failed | 3 passed (5)

The 48 that stayed green are the measured proof the unit layer had zero coverage of the row key before this commit. The two integration fixtures needed no synthetic ablation: their red was the real defect, observed before the fix and green after.

Verification (all on 7ec7d8d4b4, after merging origin/main)

Both failures reproduced locally first, byte-identical to CI — including on a real MySQL 8.0.46, with the suite's own ANSI_QUOTES non-vacuity assertion passing.

metadata-protocol pnpm test Test Files 143 passed (143) Tests 1968 passed (1968)
(live-mysql cell confirmed in the run set, 0 skipped)
cli platform-migrations-arming Test Files 1 passed Tests 6 passed (6)
cli typecheck (tsc --noEmit) rc=0
runtime both tenancy-split integration files Test Files 2 passed Tests 15 passed (15)
pnpm lint eslint . --no-inline-config rc=0 (full repo scan, not narrowed)

Gate families derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (change set taken from the merge base, not a hand-written diff) — all green, quoting each gate's own verdict line:

check:nul-bytes · check:engine-double-contract · check:where-matcher · check:cross-package-test-inputs · check:test-source-alias · check:objectql-double-limit · check:query-options-erasure · check:durability-log-level · check:type-check-coverage · check:changeset-gate-self-tests · check:page-declaration-shape · check:published-files · check:slot-lookup · check:type-source-resolution · check:objectui-changeset · check:i18n · check:i18n-coverage · check-adr-0087-registration · check-changeset-no-major · check-ci-filter-parity · check-comment-mask-adoption · check-empty-changeset · check-plugin-teardown-shape · release-rehearsal-clone --self-test

check:i18n and check:i18n-coverage first exited 1 with PREREQUISITE NOT MET / COULD NOT MEASURE — an unbuilt CLI and unbuilt example dependencies in this worktree, which measures nothing rather than failing. Recorded as not measured, then actually measured after building: check-i18n-coverage: OK (12 config(s), 602 baselined untranslated string(s), none new).

Not run locally: check:type-check-debt --re-measure, which needs the whole workspace closure built. CI runs it.

I did not touch lines 1016–1025 of seed-tenancy-backfill.ts, and no test was skipped, disabled or weakened.

Generated by Claude Code


Generated by Claude Code

@os-warrenClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review of the repair — accepted. My diagnosis was wrong on all three counts; here is the correction.

I posted a hypothesis with this dispatch and it does not survive. Recording that plainly, because the dispatch brief told the dev to "confirm or refute rather than assume" and refuting it was the right outcome.

1. My hypothesis is refuted. I said the "is the org-scoped row absent?" probe and the primary key were different keys. They are not — probe, INSERT and UPDATE already address the same key. Nothing was wrong with the repair's key handling.

2. My dialect framing was wrong. I wrote that the {field}-scope pin "drives SQLite, and this is Postgres", and pushed the dev to close "the dialect gap". The defect reproduces identically on SQLite (the cli fixture) and MySQL. It was never a dialect gap.

3. My smoking gun was a deliberate probe — and I had warned about exactly that trap in the same comment. The _objectstack_sequences_pkey duplicate-key lines belong to sql-driver-autonumber-cold-race.test.ts, whose own docblock says it exists because "the first CONCURRENT autonumber insert into a COLD object" races, and that getNextSequenceValue"handled the first-insert race by catching the unique [constraint violation]". It provokes that violation on purpose. I flagged its sibling os8269_poison_probe_pkey as deliberate in the same breath and then read the adjacent line — same os8269_ prefix, same service log — as evidence. Naming the trap is not the same as avoiding it.

The real root cause is better than mine, and it is not in the repair

Two integration fixtures hand-seeded key_hash as an invented string'h1'/'h2', 'h_global'/'h_org'. That was inert while the migration addressed counter rows by (object, field, tenant_id): nothing read the column, so any string did. This PR addresses the destination by key_hash, the key the table is actually keyed on — so an invented hash describes a table no install can hold. The probe correctly reports the org row absent, the INSERT fires, and a second counter lands beside the first.

The production invariant was measured, not read: a real SqlDriver allocating one number stores key_hash=b0f1b781…, and sha256(object ␟ tenant_id ␟ field ␟ scope) recomputes it byte-identically. createSequencesTable declares key_hash .notNullable().primary() and it is the table's only key — no unique index stands behind the natural key — and ensureSequencesKeyHashShape recomputes the same hash for every legacy row it migrates. So the PR's production code was correct as written, and the fix is three test files plus one export keyword. Verified: the only non-test file in the whole branch is seed-tenancy-backfill.ts, and #12395's band 1016–1025 is still untouched after the merge (earliest old-side line: 1083).

The finding under the finding

Every fake seam in the unit layer dispatches on statement shapesql.includes('"key_hash" = ?') — and hands back its one row without reading the parameter. That models a database in which every key addresses the same row, so the INSERT-vs-UPDATE decision cannot fail there. The UPDATE branch reached CI only through the two wrong fixtures.

The new pin replaces that with a real keyed store: rows live under their own key_hash, the probe answers by the parameter given, and INSERT refuses a key already present, because that is what a primary key does. The ablation's number is the part to keep: mutating the destination probe's key reds exactly the three new cases while 48 stay green — measured proof the unit layer had zero coverage of the row key before this commit. That is a sharper result than the dialect axis I sent them after, and it is the kind of gap that survives any number of new dialect cells.

Also right: the two integration fixtures got no synthetic ablation, because their red was the real defect — observed before the fix, green after. Manufacturing one would have been theatre.

Base branch ruled out first, as the discipline requires

Recent completed CI runs on main (32618554062, 32618184038, 32617565109) all conclude success. Neither job is red on base, so both failures were this PR's. That check came before the edit, not after.

No assertion was changed, weakened, skipped or quarantined — the two failing assertions were already correct and now pass against fixtures that are realistic.

One judgement call surfaced for the maintainer, not decided

Should the migration address the destination row by its storedkey_hash (found via the natural key), the way it already retires the '__global__' row, rather than by a recomputed hash? Declined on measurement, and the asymmetry is now deliberate and on the record rather than an oversight. The argument is asymmetric in the right direction: under the production invariant the two are identical, and where they could ever disagree, recomputing writes a row the driver can read at the merged mark — while probing by natural key and updating by stored hash would raise a row the driver cannot read, sending it back into MAX(data) to re-issue the burned number. That is this card's own damage, reintroduced by the safer-sounding option.

Declared, still owed to CI

check:type-check-debt --re-measure (refuses on an unbuilt closure) and the rest of the packages/cli shard — only the failing file plus cli typecheck and a full-repo eslint were run locally. check:i18n / check:i18n-coverage first exited 1 with PREREQUISITE NOT MET / COULD NOT MEASURE; recorded as not measured, then built and genuinely green. A gate refusing to measure is not a red.

Auto-merge stays armed. #12395 unblocks when this lands.


Generated by Claude Code

@os-warren
os-warren added this pull request to the merge queueAug 26, 2026
Merged via the queue into main with commit da1126aAug 26, 2026
33 checks passed
@os-warren
os-warren deleted the claude/issue-12394-autonumber-highwater branch August 26, 2026 13:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-warren@claude