Skip to content

fix(driver-sql): a shadow-carried UNIQUE is not index drift, and its remedy dropped the constraint - #13055

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-13015-shadow-unique-index-drift
Aug 29, 2026
Merged

fix(driver-sql): a shadow-carried UNIQUE is not index drift, and its remedy dropped the constraint#13055
os-zhuang merged 1 commit into
mainfrom
claude/issue-13015-shadow-unique-index-drift

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #13015

A healthy #11627 hash-shadow-carried UNIQUE was reported as destructive index
drift, and following the remedy the differ printed would have dropped the
constraint. Both halves are fixed here.

The defect, reproduced

diffManagedIndexes compared the declared key against the columns an index
physically KEYS. A shadow-carried UNIQUE keys exactly one driver-owned
VARBINARY(32) generated column, so the comparison could never match. Neither
escape hatch covered it: isRuntimeManagedIndex sees no expressions (the shadow
is a plain column key) and the runtimeCreatedIndexes ledger only records DDL
issued through the public execute() seam.

Why the remedy was worse than the defect

recreate_index drops the UNIQUE by name and re-runs the additive sync. The
sync retakes the shadow route, and its ALTER TABLE ... ADD COLUMN then fails
on the generated column that SURVIVED the index drop. That is a
duplicate-COLUMN error, matched by neither the "already exists" absorb (which
spells index names) nor the unique-violation branch — so the apply ends with the
constraint dropped and not re-created. An operator following the finding's own
os migrate apply --allow-destructive advice removed a live uniqueness
guarantee.

The direction taken, and why not the weaker one

The card offered a weaker alternative: record shadow indexes as runtime-managed
so the differ leaves them alone. Rejected, and the rejection is load-bearing.
A shadow written before #12998 hashes the RAW columns, so CONCAT yields NULL
for every NULL-organization row and the rows the COALESCE(organization_id, '__global__') bucket exists to constrain are constrained by nothing (#5030's
shape). It is indistinguishable BY NAME from a healthy shadow. A blind skip
would have traded one false destructive finding for a true silent one — a class
of unenforced-constraint drift made permanently invisible.

Since #12998 the stored GENERATION_EXPRESSION carries the NULL-safe COALESCE
parts, so a real key-form comparison is available, and that is what this does.

The card also notes that half the vocabulary was already taught: the
orphan-COLUMN pass guards the shadow via isHashShadowColumn while the index it
carries was proposed for destructive rebuild. The fix makes both passes read one
vocabulary rather than special-casing the differ:

  • hashShadowColumnFor moves from SqlDriver into schema-drift.ts beside
    isHashShadowColumn; the static now delegates, so the name the sync CREATES
    and the name the differ LOOKS FOR have one definition.
  • parseHashShadowKeyParts is the one parser for the generation expression;
    hashShadowSourceColumns now calls it instead of carrying a second copy of
    the regex.
  • introspectIndexes resolves what each carrier hashes and records it on the
    PhysicalIndex; the differ compares the key the index ENFORCES
    (enforcedIndexKey) rather than the digest column it stores. Drift reports
    and plan messages name that key too.
  • createHashShadowUniqueIndex inspects a surviving shadow column instead of
    assuming it absent: reusable is re-keyed in place, stale is re-generated,
    foreign (a non-generated column of that name) is refused rather than
    dropped. hashShadowColumnState decides "same key" through
    canonicalIndexKey, the differ's own identity, so the sync and the differ
    cannot answer the question differently.

A carrier whose expression cannot be read reports nothing rather than proposing
a drop it cannot reason about; the introspection enrichment swallows its own
failure so the read's other consumers (upsert conflict targets, the boot's
index-presence probe) cannot start failing because a second query did.

The pin, in the destructive direction

packages/drivers/driver-sql/src/sql-driver-13015-shadow-carried-index-drift.test.ts.
Every "no finding" assertion carries a colocated positive control in the same
diffManagedIndexes call — a genuinely drifted index that must still be
reported — so silence can never be read as the differ having gone quiet. The
control shares no key column and no name fragment with the shadow fixtures.

The live-MySQL cell reads the PHYSICAL catalog (information_schema), never the
differ's report about itself, and asserts the declared UNIQUE is present and
carried by its shadow BEFORE concluding anything from an empty drift list. It
covers: a fresh sync reporting no index drift; applying every reported entry
with allowDestructive and the constraint still standing and still enforcing; a
manually dropped index whose surviving shadow column is re-keyed; and a
raw-column shadow that is reported, then repaired to the COALESCE form.

Ablation, measured. Two anchored mutations restore the pre-fix differ
(enforcedIndexKey always returns the physical key; the unresolved-carrier
guard disabled). Mutation proven on disk in both directions — injected marker
OS13015_ABLATION count 2, deleted anchor count 0, blob moved
39f7154227 to 11a19f78f9. Ablated run: 5 of 6 tests RED. The entry the
pre-fix differ produces for a healthy shadow-carried unique reproduces the
card's measured output exactly:

kind: index_mismatch · severity: error · category: destructive
expected: UNIQUE (COALESCE(organization_id, '__global__'), v)
actual:   UNIQUE (uniq_os13015_probe_organization_id_v__hash)
op: recreate_index (columns [organization_id, v], nullSafeColumns [organization_id])
message: "... the additive sync skips it by name, so it must be rebuilt. Creating
the UNIQUE index can fail on existing duplicates: os migrate apply --allow-destructive."

Restored byte-identically: worktree blob back to 39f7154227, marker count 0,
git diff HEAD empty, tests green again. No rebuild was needed for the
ablation and none is claimed: the suite imports ../src/index.js and
./schema-drift.js, both source paths inside the package, so vitest resolves
the mutated source directly.

The stale-shadow pin earned its place immediately: it caught a real defect in
the first cut of this fix, where the fail-safe guard was written unconditionally
and silently demoted the whole change to the blind skip argued against above —
green, quiet, and the exact trade this PR exists to refuse.

Verification

All at 2b1593e10, the final commit.

  • pnpm --filter @objectstack/driver-sql test — 145 files passed, 9 skipped;
    2219 tests passed, 131 skipped.
  • pnpm --filter @objectstack/driver-sql typecheck (tsc --noEmit) — exit 0.
    Not assumed to cover the new tests: tsc --listFiles reports the new test
    file in the program (1 hit, with schema-drift.ts as the positive control).
  • pnpm lint (repo-wide eslint . --no-inline-config) — exit 0 in 63s. No
    narrowing was needed, so none is claimed.
  • Gate family re-derived at the final commit with
    node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack
    (change set: 4 paths, all committed) and the union run there: 27 gates green,
    including check:driver-conformance, check:test-source-alias,
    check:engine-double-contract, check:cross-package-test-inputs,
    check:dispatcher-error-vocabulary, check:nul-bytes, and the four
    changeset families.
  • check:type-check-coverage green; check:type-check-debt --re-measure green
    after building the workspace closure — 31 ledger entries re-measured, 1570 raw
    tsc errors, none above its recorded number.
  • scripts/pm/check-half-states.mjs exited 3 = PREREQUISITE NOT MET (no
    GitHub credential in this container). NOT MEASURED, not a red, and unrelated
    to this diff.

Scope notes

Clause-②, asked out loud and answered against the actual diff: NO.
packages/drivers/** is not the contract face and this diff does not widen a
public surface — an earlier cut exported the shadow vocabulary from
src/index.ts and that was reverted, because nothing outside this package
consumes a hash shadow. src/index.ts is byte-identical to main. What the
diff DOES change is what os migrate plan reports and what os migrate apply
does to a production constraint, which is why the question is asked rather than
assumed away; but the accept/reject behaviour of any authored contract is
untouched, no metadata key changes meaning, and no expectation was re-baselined
to match new output. The one existing behaviour that shifts is the actual
string and message on shadow-carried findings, which now name the enforced key
instead of the digest column.

Changeset: patch, argued. No public surface change (verified: index.ts
untouched), nothing removed or renamed, so neither minor nor breaking applies.
Not "no changeset" either — AGENTS.md exempts pure bug fixes, but this one
changes operator-visible behaviour on a destructive migration path, which
belongs in the release notes.

Serial queue: this fix is NOT confineable to schema-drift.ts. The differ
half alone would be, but it would be the blind skip. The real key-form
comparison needs the GENERATION_EXPRESSION read in introspectIndexes, and
the surviving-generated-column handling is in createHashShadowUniqueIndex
both in sql-driver.ts. #12999 stays serial behind this card.

Generated by Claude Code


Generated by Claude Code

…remedy dropped the constraint

The index differ compared declared columns against the columns an index
physically KEYS. A #11627 hash-shadow-carried UNIQUE keys exactly one
driver-owned VARBINARY(32) generated column, so the comparison could never
match and a clean boot reported the index it had just created as
destructive `recreate_index` drift.

Following that remedy removed the constraint: the drop-by-name leaves the
generated column behind, the re-sync's shadow `ADD COLUMN` fails on the
survivor with a duplicate-COLUMN error that neither the "already exists"
absorb nor the unique-violation branch matches, and the apply ends with the
UNIQUE dropped and not re-created.

Both passes now read one vocabulary instead of special-casing the differ:
the shadow name derivation moves next to `isHashShadowColumn`, introspection
resolves what the shadow HASHES from the stored GENERATION_EXPRESSION, the
differ compares the ENFORCED key, and the sync inspects a surviving shadow
column (re-key / re-generate / refuse) rather than assuming it absent.

A real key comparison, not a blanket skip: a pre-#12998 shadow hashes the RAW
columns and leaves every NULL-organization row unconstrained, and is
indistinguishable by name from a healthy one. It stays reported, as the
ADR-0120 D4 tightening it is.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LZbWd2jNV1FErXTPSS4Dry
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql, touching 16 documentable anchor(s).

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

  • content/docs/data-modeling/drivers.mdx (via SqlDriver (symbol))
  • content/docs/data-modeling/index.mdx (via SqlDriver (symbol))
  • content/docs/plugins/packages.mdx (via SqlDriver (symbol))
  • content/docs/protocol/kernel/index.mdx (via SqlDriver (symbol))
  • content/docs/protocol/kernel/lifecycle.mdx (via SqlDriver (symbol))
  • content/docs/protocol/objectql/query-syntax.mdx (via SqlDriver (symbol))

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

  • content/docs/releases/v17.mdx (via SqlDriver (symbol))

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
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • 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 — 9 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 df59de0d69130fee44602f4cb1368b07639a886cpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 397c11c2150236505ffd0825ef1ab43074de649b — the merge of head 2b1593e104579bc4c0dbeb826796bcb2ad8863d5 into base df59de0d69130fee44602f4cb1368b07639a886c, 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 397c11c2150236505ffd0825ef1ab43074de649b && git checkout 397c11c2150236505ffd0825ef1ab43074de649b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin df59de0d69130fee44602f4cb1368b07639a886c 2b1593e104579bc4c0dbeb826796bcb2ad8863d5 && git checkout -B drift-repro df59de0d69130fee44602f4cb1368b07639a886c && git merge --no-ff 2b1593e104579bc4c0dbeb826796bcb2ad8863d5

node scripts/docs-audit/affected-docs.mjs --json df59de0d69130fee44602f4cb1368b07639a886c

⚠️ 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 df59de0d69130fee44602f4cb1368b07639a886c → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants