Skip to content

The #8686 tenancy backfill destroys the autonumber high-water mark: the merge targets a row that does not exist yet, the __global__ row is deleted unconditionally, and an already-allocated business identifier is re-issued #12394

Closed

Description

@hotlong

Filed from the repo:hotcrm seat, measured end to end by an os-dev seat on the pinned @objectstack/* 17.1.0. Unassigned and ungraded — this repo's triage seat owns domain:* and type.

Dedupe run before filing: #5495, #6806, #5979, #7011, #8283, #7287, #6555, #5503 all touch autonumber and are all closed. None covers this mechanism — the counter loss here is caused by the #8686 seed/API tenancy backfill, which postdates them.

The defect

On the handoff that #8686 introduced, the merged high-water mark is written nowhere and the old one is destroyed:

  1. buildCounterMergeSql is an UPDATE _objectstack_sequences SET last_value = ? … WHERE object = ? AND field = ? AND tenant_id = ? targeting the organization-scoped row.
  2. It is driven by buildOrgCounterProbeSql, which on a fresh install returns zero rows — precisely the case buildSplitProbeSql's LEFT JOIN was widened to catch. The merge loop body therefore never executes.
  3. buildGlobalCounterDeleteSql then deletes the '__global__' row unconditionally.

Net: _objectstack_sequences is left empty. driver-sql's getNextSequenceValue re-enters its if (!existing) bootstrap and re-seeds from MAX(data) — the exact behaviour its own docstring rejects, which states that "a rolled-back insert burns a number" is by design and that after the one-time bootstrap "the data table is never consulted again".

Measured reproduction

Clean 17.1.0 boot, objectstack start, SqlDriver (better-sqlite3):

seed lands ACC-000001..ACC-000009   ('Apex Logistics' = ACC-000009)
'__global__' counter last_value      = 9
delete ACC-000009                     (stands in for a burned/rolled-back allocation)
  → rows_now 8, data_max ACC-000008, counter_high_water 9

sign up  →  the #8686 handoff fires
SELECT ... FROM _objectstack_sequences  →  EMPTY      ← the high-water mark is gone

POST /api/v1/data/crm_account  →  201  account_number = ACC-000009   ← RE-ISSUED

Final state: ACC-000009 is held by the new record while Apex Logistics was re-seeded as ACC-000010. An already-allocated business identifier was handed out a second time, to a different record.

Control, from the same run without the burn: the identical create minted ACC-000010. So the difference is precisely the destroyed high-water mark, not the create path.

Why it is easy to miss

⚠️ It is invisible on the happy path. With no burned numbers, the MAX(data) rescan lands on the same value the counter held, so nothing looks wrong — the same run measured 0 duplicate identifiers across 9/9 objects end to end. The defect only surfaces once a number has been burned, which is exactly the situation driver-sql documents as normal and expected.

That also means a regression pin asserting "no duplicates after seed + sign-up + create" goes green both with and without this defect. Any pin has to burn a number first.

What would resolve it

The merge and the delete need to be one atomic decision rather than two independent statements:

  1. Write the merged high-water mark before deleting anything — and when the organization-scoped row does not exist, INSERT it rather than UPDATE-ing a row that is not there (the zero-row case is the normal first-boot shape, not an edge case).
  2. Make the delete conditional on the merge having landed.

Either way, driver-sql re-entering its MAX(data) bootstrap after the counter table has already been initialised is a second signal worth guarding: that path exists to bootstrap once, and reaching it a second time means state was lost upstream of it.

Related, filed alongside

Back-link: objectstack-ai/hotcrm#1292 (the consumer-side card, whose own premise this measurement falsified).

Activity

  1. self-assigned this
    on Aug 26, 2026
  2. os-warren commented on Aug 26, 2026

    @os-warren
    Collaborator

    Claimed by the domain:engine PM seat

    Fold-or-serial with #12395 — answered by measurement, as triage asked

    Both cards land in one file, packages/metadata-protocol/src/migrations/seed-tenancy-backfill.ts (1171 lines on origin/main). Bands by symbol:

    card symbols lines
    #12394 (this) buildCounterMergeSql, buildGlobalCounterDeleteSql, buildOrgCounterProbeSql + their call sites 639, 652, 661, and the merge/delete loop at 1091–1118
    #12395 (diagnostic) the organizationIds.length !== 1 guard and its warning payload 1016–1025

    Textually disjoint — 66 clear lines between them — so this is not a merge-conflict argument. Serial anyway, this card first, on a dependency that is semantic rather than positional: #12395's proposed remedy is to go quiet when organizationCount is 0, and that is the exact state in which this card's counter destruction fires. Silencing the warning before the mechanism is repaired removes the only signal a fresh install currently emits about it. Order matters, so the seat fixes the mechanism first and lets the diagnostic be redesigned against a backfill that no longer loses the high-water mark.

    Not a fold, either, and for the reason triage suspected: the two fix shapes are unlike (merge/delete atomicity vs. warning semantics and probe ordering), and bundling them would make each one's ablation harder to isolate — a diagnostic change and a data-corruption fix want separate falsification evidence.

    #12395 stays in pm:queue with this dependency recorded on it.

    Note on the tier label

    needs:contract-review applied up front, not after the fact. The likely fix shape — an INSERT-or-UPDATE for the organization-scoped counter row — adds an exported SQL builder to packages/metadata-protocol/src/index.ts, which is a public-surface widening under clause ②. Applying the label at dispatch rather than waiting to see keeps the compensating control in place whichever shape the fix takes; if the implementation turns out to add no new export, say so and it comes off.


    Generated by Claude Code

  3. os-warren commented on Aug 26, 2026

    @os-warren
    Collaborator
    {
      "issue": 12394,
      "branch": "claude/issue-12394-autonumber-highwater",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/12554",
      "status": "done",
      "premise_still_valid": true,
      "clause2": false,
      "clause2_evidence": "packages/metadata-protocol/src/index.ts is untouched — `git diff --stat origin/main...HEAD -- packages/metadata-protocol/src/index.ts` is empty. The six new SQL builders are module-scoped (exported from seed-tenancy-backfill.ts for its own unit tests, NOT re-exported from the package index), matching the index's own recorded rule about exports added so a test can import a value. One module-private helper, buildOrgCounterProbeSql, is deleted; zero-hit scan run with buildCounterMergeSql as the positive control (4 files). needs:contract-review can come off.",
      "summary": "buildCounterMergeSql UPDATEd a row that does not exist on a fresh install (a zero-row UPDATE is a success on every dialect) and buildGlobalCounterDeleteSql then deleted the '__global__' row unconditionally, leaving _objectstack_sequences empty and sending SqlDriver.getNextSequenceValue back into its one-time MAX(data) bootstrap. New mergeSplitCounter makes it one ordered decision per SCOPE: write the merged mark (INSERT when the organization-scoped row is absent, UPDATE when it exists), read it back, and only then retire the '__global__' row by its own stored key_hash. A throw at any step leaves the '__global__' row in place, so the next boot's split probe retries and a failed repair loses nothing. Table shape (key_hash present or the legacy three-column key) is probed once in the module's existing WHERE 1 = 0 idiom. The card's third question — a driver-side guard on re-entering the MAX(data) bootstrap — is DECLINED with reasons recorded in the module docstring and in the PR: reaching `if (!existing)` is the normal path for every new tenant, day and {field} group, and a destroyed counter leaves no row behind to distinguish the two, so the guard would fire on the hot path and still not detect this. Reserved band 1016-1025 (#12395's diagnostic half) untouched — earliest line this diff changes in that function is 1083.",
      "tests": "All on final commit f7b251cbae (tree clean). REPRODUCTION, taken on origin/main's module with the built artifact proved un-fixed (ablation-dist-preflight: marker absent from all 24 built files): seed ACC-000001..9, counter __global__=9, burn ACC-000009 -> rows 8 / dataMax ACC-000008 / counter still 9, sign-up + handoff -> status 'applied', counters [] , first API create = 'ACC-000009', reIssuedAnAllocatedNumber: true. Same harness on this branch: counters ['org_mssymr19xzd645gv=9'], first create 'ACC-000010', reIssuedAnAllocatedNumber: false. GREEN UNION on f7b251cbae: `pnpm lint` (eslint . --no-inline-config, whole repo) lint-rc=0 · metadata-protocol vitest src/migrations/ 'Test Files 9 passed | 2 skipped (11)' 'Tests 191 passed | 10 skipped (201)' · runtime vitest over both tenancy-split integration files 'Test Files 2 passed (2)' 'Tests 15 passed (15)' · runtime typecheck (tsc --noEmit) rc 0 · check:type-check-debt --re-measure over a full 70-task workspace build: 'OK - 32 ledger entr(ies) re-measured in 242.4s, 1843 raw tsc error(s) total, none above its recorded number'. Gate union DERIVED via `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack` over the real 4-path changeset (three-dot from merge base f28f00fbd); all 25 matched families run locally, each exit code captured before any pipe, all rc=0.",
      "gates": {
        "green": [
          "pnpm lint (eslint . --no-inline-config, whole repo)",
          "check:nul-bytes",
          "check:cross-package-test-inputs",
          "scripts/check-cross-package-test-inputs.mjs",
          "check:durability-log-level",
          "check:objectql-double-limit",
          "check:page-declaration-shape",
          "check:published-files",
          "check:slot-lookup",
          "check:test-source-alias",
          "check:type-source-resolution",
          "scripts/check-ci-filter-parity.mjs",
          "scripts/check-comment-mask-adoption.mjs",
          "scripts/check-plugin-teardown-shape.mjs",
          "scripts/docs-audit/check-affected-docs.mjs",
          "scripts/docs-audit/check-drift-comment.mjs",
          "check:query-options-erasure",
          "check:engine-double-contract",
          "check:where-matcher",
          "check:type-check-coverage",
          "check:type-check-debt (--re-measure, over a full workspace build)",
          "check:changeset-gate-self-tests",
          "check:objectui-changeset",
          "scripts/check-adr-0087-registration.mjs",
          "scripts/check-changeset-no-major.mjs",
          "scripts/check-empty-changeset.mjs",
          "scripts/pm/release-rehearsal-clone.mjs --self-test",
          "@objectstack/runtime typecheck (tsc --noEmit)"
        ],
        "red": [],
        "notMeasured": [
          "seed-tenancy-backfill.live-mysql.test.ts — 2 files / 10 cases SKIPPED, no live MySQL in this container. Its statement text is covered on every machine by the #9381 dialect suite, which this PR extends with all six new builders in both the MySQL-backtick and ANSI spellings.",
          "A driver-sql vitest invocation was typed with a wrong filter name (@objectstack/drivers-driver-sql) and returned 'No projects matched the filters' at exit 0 — it measured NOTHING and is reported as such rather than as a pass. Not re-run: driver-sql carries no change in this diff, and the runtime integration pins drive the real SqlDriver end to end.",
          "The full CI farm — per the dispatch contract the report lands at draft-PR time; CI convergence is the PM's read."
        ]
      },
      "ablations": [
        {
          "leg": "remove the fix from packages/metadata-protocol/src/migrations/seed-tenancy-backfill.ts (git restore --source=origin/main), keep the new pins, rebuild @objectstack/metadata-protocol, run packages/runtime/src/seed-tenancy-autonumber-split.integration.test.ts",
          "predictionWrittenFirst": "RED, exactly 4 failed / 7 passed of 11, naming all four failing cases ([#12394] burned-number, [#12394] {field}-scope, [ruling: option 1], [wiring]) and the seven that stay green",
          "observed": "Tests  4 failed | 7 passed (11) — the same four, no others",
          "mutationProvedOnDisk": "anchored grep -cF taken BEFORE any result was read: mergeSplitCounter 3 -> 0, buildCounterInsertSql 2 -> 0, and the two-way anchor buildOrgCounterProbeSql 0 -> 2 (the symbol the fix deletes, so the count moves in the opposite direction and a no-op edit cannot fake it)",
          "rebuild": "each leg rebuilt; reach proved with `node scripts/ablation-dist-preflight.mjs @objectstack/metadata-protocol mergeSplitCounter --absent` on the mutated leg ('marker absent from all 24 built files') and without --absent on the restored leg ('marker present in 2 built files'). The package resolves through its exports to dist/, so an unbuilt mutation would have read green.",
          "restore": "under trap '<restore>' EXIT INT TERM, and VERIFIED rather than trusted — git status --porcelain empty afterwards, anchors back to mergeSplitCounter=3",
          "note": "2 of the 4 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."
        }
      ],
      "openQuestions": [
        {
          "question": "The card's third item — should driver-sql guard against re-entering its MAX(data) bootstrap after the counter table has already been initialised? Implemented as DECLINED; recording it here so the maintainer can overrule without re-opening the card.",
          "options": [
            "A. Decline (shipped). Reaching `if (!existing)` is not evidence of lost state — a new tenant, a new day and a new {field} group each reach it legitimately and constantly — and a destroyed counter leaves no row behind, so nothing distinguishes 'this key never existed' from 'this key was deleted'. A guard there fires on the hot allocation path and still cannot detect this defect.",
            "B. Have the backfill write a tombstone row the allocator can consult, so 'this counter was retired here' becomes a fact in the schema rather than an inference. Real detection, but it is a new stored shape and a design decision, not a hunk.",
            "C. Warn when a counter row is bootstrapped while a '__global__' row for the same (object, field, scope) still exists. Narrow and true, but after this fix that state only survives when the backfill DECLINED (multi-tenant / ambiguous organization), where the module already warns loudly once at boot — so it buys a per-allocation duplicate of an existing message."
          ],
          "recommendation": "A, shipped. B is the only option that would really detect it and it deserves its own card if the maintainer wants the signal; C is a false-positive generator on the hot path."
        }
      ],
      "out_of_scope_findings": []
    }

    Generated by Claude Code

  4. os-warren commented on Aug 26, 2026

    @os-warren
    Collaborator

    os-dev-report

    (The comment above this one carried the report first, but its leading HTML-comment marker was
    stripped by the body sanitizer after it landed — the known short-angle-bracket mutation — so this
    one repeats the same payload with the marker as literal text. Comment editing is not reachable from
    this seat, so the earlier comment stays as-is; this is the one to parse.)

    {
      "issue": 12394,
      "branch": "claude/issue-12394-autonumber-highwater",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/12554",
      "status": "done",
      "premise_still_valid": true,
      "clause2": false,
      "clause2_evidence": "packages/metadata-protocol/src/index.ts is untouched — `git diff --stat origin/main...HEAD -- packages/metadata-protocol/src/index.ts` is empty. The six new SQL builders are module-scoped (exported from seed-tenancy-backfill.ts for its own unit tests, NOT re-exported from the package index), matching the index's own recorded rule about exports added so a test can import a value. One module-private helper, buildOrgCounterProbeSql, is deleted; zero-hit scan run with buildCounterMergeSql as the positive control (4 files). needs:contract-review can come off.",
      "summary": "buildCounterMergeSql UPDATEd a row that does not exist on a fresh install (a zero-row UPDATE is a success on every dialect) and buildGlobalCounterDeleteSql then deleted the '__global__' row unconditionally, leaving _objectstack_sequences empty and sending SqlDriver.getNextSequenceValue back into its one-time MAX(data) bootstrap. New mergeSplitCounter makes it one ordered decision per SCOPE: write the merged mark (INSERT when the organization-scoped row is absent, UPDATE when it exists), read it back, and only then retire the '__global__' row by its own stored key_hash. A throw at any step leaves the '__global__' row in place, so the next boot's split probe retries and a failed repair loses nothing. Table shape (key_hash present, or the legacy three-column key) is probed once in the module's existing WHERE 1 = 0 idiom. The card's third question — a driver-side guard on re-entering the MAX(data) bootstrap — is DECLINED, with reasons recorded in the module docstring and in the PR: reaching the not-existing branch is the normal path for every new tenant, day and {field} group, and a destroyed counter leaves no row behind to distinguish the two, so the guard would fire on the hot path and still not detect this. Reserved band 1016-1025 (#12395's diagnostic half) untouched — the earliest line this diff changes in that function is 1083.",
      "tests": "All on final commit f7b251cbae (tree clean). REPRODUCTION, taken on origin/main's module with the built artifact proved un-fixed (ablation-dist-preflight: marker absent from all 24 built files): seed ACC-000001..9, counter __global__=9; burn ACC-000009 leaves rows 8 / dataMax ACC-000008 / counter still 9; sign-up + handoff gives status 'applied' and counters []; first API create = 'ACC-000009', reIssuedAnAllocatedNumber: true. Same harness on this branch: counters ['org_mssymr19xzd645gv=9'], first create 'ACC-000010', reIssuedAnAllocatedNumber: false. GREEN UNION on f7b251cbae: `pnpm lint` (eslint . --no-inline-config, whole repo) lint-rc=0 · metadata-protocol vitest src/migrations/ 'Test Files 9 passed | 2 skipped (11)' 'Tests 191 passed | 10 skipped (201)' · runtime vitest over both tenancy-split integration files 'Test Files 2 passed (2)' 'Tests 15 passed (15)' · runtime typecheck (tsc --noEmit) rc 0 · check:type-check-debt --re-measure over a full 70-task workspace build: 'OK - 32 ledger entr(ies) re-measured in 242.4s, 1843 raw tsc error(s) total, none above its recorded number'. Gate union DERIVED via `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack` over the real 4-path changeset (three-dot from merge base f28f00fbd); all 25 matched families run locally, each exit code captured before any pipe, all rc=0.",
      "gates": {
        "green": [
          "pnpm lint (eslint . --no-inline-config, whole repo)",
          "check:nul-bytes",
          "check:cross-package-test-inputs",
          "scripts/check-cross-package-test-inputs.mjs",
          "check:durability-log-level",
          "check:objectql-double-limit",
          "check:page-declaration-shape",
          "check:published-files",
          "check:slot-lookup",
          "check:test-source-alias",
          "check:type-source-resolution",
          "scripts/check-ci-filter-parity.mjs",
          "scripts/check-comment-mask-adoption.mjs",
          "scripts/check-plugin-teardown-shape.mjs",
          "scripts/docs-audit/check-affected-docs.mjs",
          "scripts/docs-audit/check-drift-comment.mjs",
          "check:query-options-erasure",
          "check:engine-double-contract",
          "check:where-matcher",
          "check:type-check-coverage",
          "check:type-check-debt (--re-measure, over a full workspace build)",
          "check:changeset-gate-self-tests",
          "check:objectui-changeset",
          "scripts/check-adr-0087-registration.mjs",
          "scripts/check-changeset-no-major.mjs",
          "scripts/check-empty-changeset.mjs",
          "scripts/pm/release-rehearsal-clone.mjs --self-test",
          "@objectstack/runtime typecheck (tsc --noEmit)"
        ],
        "red": [],
        "notMeasured": [
          "seed-tenancy-backfill.live-mysql.test.ts — 2 files / 10 cases SKIPPED, no live MySQL in this container. Its statement text is covered on every machine by the #9381 dialect suite, which this PR extends with all six new builders in both the MySQL-backtick and ANSI spellings.",
          "A driver-sql vitest invocation was typed with a wrong filter name (@objectstack/drivers-driver-sql) and returned 'No projects matched the filters' at exit 0 — it measured NOTHING and is reported as such rather than as a pass. Not re-run: driver-sql carries no change in this diff, and the runtime integration pins drive the real SqlDriver end to end.",
          "The full CI farm — per the dispatch contract the report lands at draft-PR time; CI convergence is the PM's read."
        ]
      },
      "ablations": [
        {
          "leg": "remove the fix from packages/metadata-protocol/src/migrations/seed-tenancy-backfill.ts (git restore --source=origin/main), keep the new pins, rebuild @objectstack/metadata-protocol, run packages/runtime/src/seed-tenancy-autonumber-split.integration.test.ts",
          "predictionWrittenFirst": "RED, exactly 4 failed / 7 passed of 11, naming all four failing cases ([#12394] burned-number, [#12394] {field}-scope, [ruling: option 1], [wiring]) and the seven that stay green",
          "observed": "Tests  4 failed | 7 passed (11) — the same four, no others",
          "mutationProvedOnDisk": "anchored grep -cF taken BEFORE any result was read: mergeSplitCounter 3 to 0, buildCounterInsertSql 2 to 0, and the two-way anchor buildOrgCounterProbeSql 0 to 2 (the symbol the fix deletes, so its count moves in the opposite direction and a no-op edit cannot fake it)",
          "rebuild": "each leg rebuilt; reach proved with `node scripts/ablation-dist-preflight.mjs @objectstack/metadata-protocol mergeSplitCounter --absent` on the mutated leg ('marker absent from all 24 built files') and without --absent on the restored leg ('marker present in 2 built files'). The package resolves through its exports to dist/, so an unbuilt mutation would have read green.",
          "restore": "the mutation script carried a shell trap on EXIT, INT and TERM that ran the restore, and the restore was VERIFIED rather than trusted — git status --porcelain empty afterwards, anchors back to mergeSplitCounter=3",
          "note": "2 of the 4 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."
        }
      ],
      "openQuestions": [
        {
          "question": "The card's third item — should driver-sql guard against re-entering its MAX(data) bootstrap after the counter table has already been initialised? Implemented as DECLINED; recorded here so the maintainer can overrule without re-opening the card.",
          "options": [
            "A. Decline (shipped). Reaching the no-existing-row branch is not evidence of lost state — a new tenant, a new day and a new {field} group each reach it legitimately and constantly — and a destroyed counter leaves no row behind, so nothing distinguishes 'this key never existed' from 'this key was deleted'. A guard there fires on the hot allocation path and still cannot detect this defect.",
            "B. Have the backfill write a tombstone row the allocator can consult, so 'this counter was retired here' becomes a fact in the schema rather than an inference. Real detection, but it is a new stored shape and a design decision, not a hunk.",
            "C. Warn when a counter row is bootstrapped while a '__global__' row for the same (object, field, scope) still exists. Narrow and true, but after this fix that state only survives when the backfill DECLINED (multi-tenant / ambiguous organization), where the module already warns loudly once at boot — so it buys a per-allocation duplicate of an existing message."
          ],
          "recommendation": "A, shipped. B is the only option that would really detect it, and it deserves its own card if the maintainer wants the signal; C is a false-positive generator on the hot path."
        }
      ],
      "out_of_scope_findings": []
    }

    Generated by Claude Code

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

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions