Skip to content

fix(driver-sql): collect the hash shadow a retired index leaves behind - #13084

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-13056-orphan-shadow-column-cleanup
Aug 29, 2026
Merged

fix(driver-sql): collect the hash shadow a retired index leaves behind#13084
os-zhuang merged 2 commits into
mainfrom
claude/issue-13056-orphan-shadow-column-cleanup

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #13056

isHashShadowColumn's docblock is why the orphan-COLUMN drift pass skips a #11627 hash shadow, and it stated what happens instead, verbatim: the column "is then cleaned up by the index's own removal path, not by a blind column drop". There was no such path.

Premise, re-measured on arrival

Measured on this branch's base ebcc34e89, after #13015 and #12999 landed. Every claim on the card still reads true:

claim reading
three DROP COLUMN statement sites in sql-driver.ts :10885 and :10915 in the ordinary drop_column drift op, :14355 inside createHashShadowUniqueIndex (a create path). :14354 is a comment line, not a fourth site
dropIndexIfExists never touches a column ✅ its whole statement family is ALTER TABLE .. DROP CONSTRAINT, DROP INDEX IF EXISTS, ALTER TABLE .. DROP INDEX
the docblock still asserts the cleanup ✅ 1 hit for "cleaned up by the index" in schema-drift.ts; positive control isHashShadowColumn → 4 hits in the same file
hashShadowColumnFor is ONE derivation schema-drift.ts defines it, SqlDriver.hashShadowColumnFor delegates. Used, never re-derived

So when metadata stopped declaring a shadow-carried UNIQUE, diffManagedIndexes step 3 reported the index as an orphan, os migrate apply --allow-destructive dropped it, and the VARBINARY(32) STORED generated column survived keyed by nothing — while the orphan-column pass declined to report it forever, exactly as designed. A STORED generated column is recomputed and written on every INSERT and on every UPDATE touching its source columns.

The fork: (b), and the reason is a THIRD caller the card does not name

The card offered (a) drop the shadow inside the index removal path, or (b) give the drop_index op its own shadow-aware step. This is (b), and the decisive evidence is that dropIndexIfExists has three callers, not two:

call site line terminal?
replace_unique_index legacy-name drop :10781 the name goes for good
drop_index :10792 the name goes for good — this is the leak
recreate_index :10819 no — drops in order to re-create under the SAME name

recreate_index is what rules (a) out. Routing it through a cleanup would drop the shadow column immediately before syncDeclaredIndexes re-creates the index, so hashShadowColumnState would report absent every time and #13015's reusable branch — which re-keys a surviving shadow in place rather than rebuilding the table around a regenerated STORED column — would become unreachable. That turns every shadow-carried index rebuild into a full table rewrite, silently, on exactly the large MySQL tables the shadow route exists for.

So the discriminator is not which caller but is this index name coming back, and only the op knows that. dropIndexIfExists cannot tell the two apart, which is precisely why the step does not belong inside it.

The legacy replace_unique_index question, answered explicitly

The card asks whether a legacy name's shadow should go with it. It deliberately does not, and the legacy path stays byte-identical — because it cannot reach a shadow at all. #13015 already added this to the legacy-detection filter in diffManagedIndexes, with a comment naming this exact reason:

// #13015: nor is a hash-shadow carrier. Its stored key is one generated
// column, so the identity comparison below already excludes it — stated
// outright because the exclusion must survive that comparison changing,
// and because `replace_unique_index` DROPS the legacy name.
if (isHashShadowCarrier(p)) return false;

diffManagedIndexes is the only producer of replace_unique_index ops, so a shadow-carried name can never enter dropIndexNames. Adding a shadow-aware step there would be enforcement for a state the producer excludes by construction — coverage in appearance only. Per the card's instruction, that "deliberately does not" is pinned rather than merely asserted, from both ends.

The change

SqlDriver.dropOrphanedHashShadowColumn(table, indexName), called by the drop_index op after the index drop. Ownership is established, never assumed — the shape of #13015's foreign guard, in the removal direction. Two refusals, each leaving the column in place and naming it in a warning:

  • a column of that name that is not generated may hold user data and is not the driver's to drop;
  • a column some other index still keys is not this orphan. That second read is what turns "index first, then column" from an ordering comment into a checked precondition: it fails closed when the drop above silently did not take, and it declines to let a DROP COLUMN remove a second index as a side effect.

An unreadable catalog degrades to leaving the column alone — the behaviour that shipped before this method existed — the same trade hashShadowColumnState makes. MySQL-only, because createHashShadowUniqueIndex refuses every other dialect.

One reporting change: a drop_index that finds the index already gone but collects the leftover column is now reported applied rather than skipped, because the apply did rewrite the table. That is the half-applied-migration state, and it is pinned.

The isHashShadowColumn docblock is corrected in the same diff — not by softening the claim, but by naming the path that now makes it true, plus the standing condition: if that method is removed or its call site moved, the orphan pass's continue becomes a leak again and must go with it.

Verification

Everything below ran on ed13cd262, which is this branch's head; the tree was clean at the time of the run.

  • pnpm --filter @objectstack/driver-sql typecheckexit 0. Not a NOT-MEASURED green: tsc --noEmit --listFiles confirms all three edited/added files are in the program (sql-driver.ts, schema-drift.ts, and the new test file — this package's tsconfig.json does not exclude *.test.ts).
  • pnpm --filter @objectstack/driver-sql exec vitest run --maxWorkers=2exit 0, Test Files 147 passed | 9 skipped (156), Tests 2241 passed | 132 skipped (2373).
  • The new file alone → Tests 12 passed | 1 skipped (13).
  • Full-repo lint, not a narrowing: eslint . --no-inline-config (the exact command pnpm lint runs) over its own population — 5407 files, 0 errors, 0 warnings, exit 0.
  • Consumer sweep, narrowed with proof. 48 packages are downstream of driver-sql, but the new member is protected, so only a subclass can see it. git grep "extends SqlDriver" across packages/**/src/** finds exactly two outside this package — SqliteWasmDriver and TursoDriver — plus one in-package testkit; and dropOrphanedHashShadowColumn collides with no existing member anywhere in packages/. Both subclasses typecheck Done with their dependency closures built. (First attempt reported TS2307: Cannot find module '@objectstack/verify' in driver-turso — an unbuilt closure, not this diff; pnpm --filter '@objectstack/driver-turso^...' build cleared it.)
  • pnpm check:nul-bytesOK, 7279 text files, no raw ASCII control bytes; plus a direct control-byte grep over the four files in this diff, which found none.
  • Derived gate family: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (change set derived by the script from the merge base, not from a hand-written diff) named 34 families. 30 exit 0. The four non-zero are prerequisite/usage refusals, not red gates, quoted from their own verdict lines:
    • check:dual-build-cjs-loadsPREREQUISITE NOT MET — this gate reads built output, and some package has no dist/ … ⛔ This is NOT a pass: nothing was measured. Its self-test passed (37 cases). Needs a full-repo pnpm build; that is CI's Build Core.
    • check:type-check-debt — the coverage half printed check-type-check-coverage: OK — 65/78 workspace packages type-checked, then --re-measure refused: cannot run: 50 workspace dependenc(ies) … have no built type entry point on disk. Same full-repo build prerequisite. check:type-check-coverage on its own is exit 0.
    • scripts/check-test-completeness.mjs — usage refusal, usage: check-test-completeness.mjs (turbo-test-log) …; CI feeds it the turbo log.
    • scripts/pm/check-half-states.mjsPREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential, and it says of itself: Nothing was swept … It is not a clean board and it is not a dirty one — it is no reading at all. A PM board sweep, not this diff. (check:pm-half-states is a different family and is exit 0.)

Ablation — mutation proven on disk, restore proven byte-identical

Both legs mutate a file whose implementation was already committed, so the restore leg has a real reference point. Each leg asserts the anchor is unique before writing, then counts the deleted text (must reach 0) and the injected text (must reach 1) on disk, and the script carries trap restore EXIT INT TERM with an absolute REPO_ROOT. Restore is git checkout HEAD -- (path), never a bare git checkout --. No rebuild is needed or claimed: the test imports ../src/index.js and ./schema-drift.js, relative paths inside the package that vitest resolves to source, never dist/ — which the ablation itself demonstrates, since an unrebuilt mutation went red.

leg mutation predicted observed
A the cleanup call replaced by const shadowDropped = false (i.e. pre-fix behaviour) RED on 3 RED on 5Tests 5 failed | 7 passed | 1 skipped
B the residual-key guard neutered (if (false)) RED on 1 RED on 1Tests 1 failed | 11 passed | 1 skipped

My prediction for leg A was wrong, in the conservative direction, and the reason is worth recording. I predicted only the three existence pins would fail and that the two REFUSES … pins would stay green, since they assert that no DROP COLUMN happened — which is also true pre-fix. They went red too, because each also asserts that the refusal was reached and logged. Removing the call means no log line, so they are existence pins as well as guard pins. Stronger than predicted, not weaker.

Byte-identity of the restore, both legs: on-disk hash returned to 0adb4e25f6cb911fd19ed71e970ea69081cd3002, equal to the HEAD blob hash for the path; git diff HEAD -- (path) empty and git status --porcelain silent for it afterwards.

What was NOT run, stated plainly

No live MySQL is reachable in this fleet — I did not measure a real server, and no reading below is asserted from one. The physical-catalog pins the card asks for are written as a live cell in the existing declareDialectCell(MYSQL_CELL, …) matrix. It is not an unrunnable cell that passes as green: without OS_TEST_MYSQL_URL it is a NAMED SKIP, which appears in the run above verbatim as

↓ sql-driver — orphan shadow column cleanup (#13056) matrix (live mysql) >
  is provisioned — set OS_TEST_MYSQL_URL to run this cell of the D-A3 driver axis

and the runner that provisions the servers — the Temporal Conformance (live PG + MySQL) job, ci.yml L836, step "Run driver-sql suite against both live servers" — sets OS_EXPECT_LIVE_DIALECT_MATRIX=1, which turns a missing URL into a failure rather than a skip. So that cell executes in CI on this PR, and nowhere in my local run did it report a pass.

Because of that, the pins that had to execute here are dialect-free and read a standing-in information_schema catalog that records every statement in order — which is how "index first, then column" is asserted as an observation of statement ordering rather than read off the source. The live cell additionally reads the real catalog and carries a colocated positive control: two shadow-carried UNIQUEs on one table, one declaration retired, the other kept — the retired column must be gone and the kept one untouched and still enforcing, in the same apply. A fix that dropped every shadow it found passes the first assertion and fails the control.

Declarations

Clause-②: no. Judged against the actual diff, not the paths. Nothing in packages/spec is touched; no metadata that was accepted is now rejected or vice versa; no schema, publish gate or validator is involved; the drift report itself is unchanged (drop_index was already what the differ emitted for this state). No export is added — the collector is protected and absent from index.ts. Asking it out loud as instructed, since this does change what os migrate apply DOES to a production table: the honest counterweight is that the destructive reach is governed by a different axis than clause-② — the drop_index op was already category: 'destructive' and already required --allow-destructive, so the operator opt-in is unchanged and only the completeness of the work it authorises changed. I record the consideration here so the PM can overrule the no on the record rather than on my summary.

Changeset: .changeset/orphan-hash-shadow-column-cleanup.md, graded patch, and the grade is argued in the changeset itself. patch because nothing new is authorable, no export is added, and no input's accept/reject verdict moves. Not minor: no capability is added. The destructive reach does not force a bump, for the reason above — what an operator sees that they did not before is a DROP COLUMN inside a migration they had already opted into, finishing the job it named.

Governed surfaces: none touched. The diff is four files — packages/drivers/driver-sql/src/sql-driver.ts, packages/drivers/driver-sql/src/schema-drift.ts, the new test file beside them, and the changeset. No docs/adr/**, no .claude/**, no skills/**, no AGENTS.md, no CLAUDE.md, and nothing under content/docs/releases/.

No existing expectation was re-baselined to match new output; none was falsified.


Generated by Claude Code

@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Aug 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql, touching 4 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/drivers/driver-sql/src/schema-drift.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

7 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/deployment/cli.mdx (via drop_index (literal))
  • 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 changed file(s) yielded no anchor (packages/drivers/driver-sql/src/schema-drift.ts) — pages documenting those are invisible to this run
  • 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 db39dfc1c9b09681474475cea6e77537ab2c89abpackageMentionDocs.

Which tree this was computed on

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

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

⚠️ 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 db39dfc1c9b09681474475cea6e77537ab2c89ab → 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