Uh oh!
There was an error while loading. Please reload this page.
fix(driver-sql): SQLite reconcile reports applied only for the ops the rebuild actually honoured - #12127
Conversation
…ebuild honoured `applyMigrationEntries` split by dialect and the two arms disagreed about what `applied` means. The in-place arm asks per entry and believes the answer; the SQLite arm called `rebuildSqliteTablePatched` and pushed EVERY entry into `applied`. That rebuild honours four op types and silently ignores the rest, so an ignored op was announced as migrated — `auto-reconciled` in the boot log, `↪ migrated` at the artifact gate — while still physically present. Next boot: detected again, reported again, "migrated" again. A loop with no failing signal. The rebuild now returns the entries it actually acted on, built in the same pass that fills the four sets it already partitioned into, so the report cannot drift from the work. The caller routes the remainder to `skipped` and logs it in the same words the in-place arm uses for an unsupported op. Reporting only — no op does anything different, and the rebuild still runs for the whole table even when it honours nothing (it re-materializes defaults and the declared index set from metadata).
📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 7c1f5d87af0030eeeab0979b045ebd74911cbb51 && git checkout 7c1f5d87af0030eeeab0979b045ebd74911cbb51
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9799ffe282d9ad3252806e6721e32b2d29ee8f9b fd1eeeb162e4db7e3afc8dadbcc9c096a78cee37 && git checkout -B drift-repro 9799ffe282d9ad3252806e6721e32b2d29ee8f9b && git merge --no-ff fd1eeeb162e4db7e3afc8dadbcc9c096a78cee37
node scripts/docs-audit/affected-docs.mjs --json 9799ffe282d9ad3252806e6721e32b2d29ee8f9b
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#11722
On SQLite,
applyMigrationEntriesreported an op as applied whenever a table rebuild ran — including the ops the rebuild is documented to ignore. This makesappliedmean the same thing on all three dialects: this op happened.The defect
SqlDriver.applyMigrationEntriessplits by dialect, and the two arms disagreed about whatappliedmeans. The in-place arm asks per entry and believes the answer —applyDriftOpInPlacereturnsfalsefor an op its dialect cannot perform, and the entry goes toskipped. The SQLite arm did not ask at all:rebuildSqliteTablePatchedhonours exactly four op types —relax_not_null,tighten_not_null,drop_column,drop_column_default— and silently ignores everything else; its own docblock already said so for the varchar ops. An ignored op was still pushed intoapplied.The failure mode is a false green, not an error. Nothing throws and nothing is skipped, so every consumer announces work that never happened:
reconcileAndWarnDriftlogsauto-reconciled {op} on {table}, and the artifact boot gate prints↪ migrated {op}. The finding is still physically present, so the next boot detects it again, reports drift again, and "migrates" it again — a loop with no failing signal anywhere in it.Why a test that passes on
mainwould have proved nothingThe gap is latent, held closed from two independent directions neither of which knows it is holding it:
enforcesVarcharLengthexcludes SQLite, so the differ never emitswiden_varchar/narrow_varcharthere; andmultiValueColumnTypeIsLoadBearingexcludes SQLite for an unrelated measured reason (a stale textual column does not corrupt the value there), so #11535'smanual_column_type_changeis never emitted there either.So the suite constructs the reachability instead of waiting for it. It substitutes exactly one thing and nothing else — the differ's dialect guard — by handing entries straight to the public
applyMigrationEntriesseam thatos migrate apply(packages/cli/src/commands/migrate/apply.ts:199) and the artifact boot gate (packages/cli/src/utils/artifact-boot-migration.ts:143) both call with differ output. Driver, dialect and database are real throughout. All five cases fail on the pre-fix tree — see the ablation below.The fix
rebuildSqliteTablePatchedreturns the entries it actually acted on, built in the same pass that fills the four column sets it already partitioned into — deliberately not a second list of op types to keep in sync, so the returned set cannot drift from the work done (honoured.push(e)is reachable only after one of the four branches took the entry; everything else hits anelse continue). The caller reports those asappliedand routes the remainder toskipped, logging it in the same sentence the in-place arm uses for an op its dialect cannot do:one greppable line across all three dialects.
@objectstack/driver-sqlite-wasmand@objectstack/driver-tursoextendSqlDriverwithout overriding either method, so both inherit the correction; neither has an override to update.What deliberately does NOT change (clause ② answer: still NO)
No op does anything different — this moves only what is reported. In particular the rebuild still runs for the whole table even when it honours nothing. It re-materializes every kept column's default (#11321, #4560) and the full declared index set from metadata (#3696), so it is not a no-op; suppressing it there would change what the reconciler DOES rather than what it says it did, which is a different question from this one. That is stated in the method's docblock so the next reader does not "optimize" it.
applied/skippedremains a reported partition consumed by log lines and CLI counts — not an accept/reject door — and no public surface widens.Declared file surface — three cards live on
sql-driver.tsEvery hunk lands inside the declared band 10100–10560 (old-side hunk headers:
10155,10196,10403,10418,10422,10428,10499).8600–9200(driver-sql: a declared field namedid/created_at/updated_atis silently discarded by initObjects — declared type, length and constraints all ignored with no diagnostic #12015 → PR fix(driver-sql): name the declared field a builtin column discards, on all three DDL paths (#12015) #12109): untouched.reconcileAndWarnDriftis called at:9176inside that band and discards the return value, so nothing there needed to move — confirmed by reading it, not assumed.13275–13900(signature/qrcodehave nomaxLengthenforcement anywhere, so they cannot join the TEXT family — a data-URI signature is refused at 255 chars and the declared bound binds nothing #11875): untouched.{@link rebuildSqliteTablePatched}docblock references at:14031and:14076were checked and are not stale: both cite the rebuild as the site that re-materializes a column's declared default, which this change does not touch. The contract that moved is the return value, which those two do not mention.packages/drivers/driver-sql/src/schema-drift.ts— themanual_column_type_changedocblock saysapplyMigrationEntriesreports the entry "skipped, never applied", measured on Postgres and MySQL. It read true as written and was silent about the one dialect where it was false; it now names SQLite reaching the same verdict by the rebuild's honoured-entry report.Verification
Gate union derived, not recalled, at the final commit
fd1eeeb162:node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack→ 14 path-matched families + 6 convention-triggered (test file added). Every one run, exit code captured before any pipe. All green; each gate's own verdict line is quoted in the report on the issue. Highlights:check:driver-conformanceOK — 45 covered cell(s), 0 in the DEBT ledger, 0 exempt.check:type-check-debt --re-measureOK — 32 ledger entr(ies) re-measured in 227.7s, 1897 raw tsc error(s) total, none above its recorded number.check:engine-double-contract390 (file, verb) row(s) held by the RETAINED ledger(no new double)check:where-matcher299 matcher(s) discovered … none newcheck:cross-package-test-inputsOK: 16 package(s) read outside themselves, all declaredpnpm lint(whole repo,eslint . --no-inline-config)0— run in full, no narrowing claimedPackages:
@objectstack/driver-sqltypecheckexit 0;test132 files / 2016 passed, 8+121 skipped. Both inheritors and the CLI consumer green —driver-sqlite-wasm25 files,driver-turso39 files,cli178 files.Ablation — direction predicted in writing before the run
Prediction (recorded before the run): restoring the caller's SQLite arm to
main's exact two lines turns the new suite RED, on theskipped-membership assertions; not "fewer diagnostics", not a reversal, because the partition is the only producer of the membership those assertions read.Mutation proved on disk by anchored counts of the text actually changed — not
git diff --stat, not an editor exit code — before any result was read:No
distis in the resolution path:packages/drivers/driver-sql/distwas absent at ablation time (shown in the run) and the suite imports./sql-driver.jsintra-package, which vitest resolves to the TS source — so no rebuild could stand between the mutation and the reading.Observed: 5 of 5 RED, the predicted direction, including the consumer-level case failing on
expected true to be false— theauto-reconciledline being logged for an op that never happened. Restore was armed withtrap … EXIT INT TERMrunninggit checkout HEAD -- {the absolute path}, then verified rather than trusted:git hash-objectof the working file18c76de812602304edb9614457060573e0ad399bequalsgit rev-parse HEAD:packages/drivers/driver-sql/src/sql-driver.ts, andgit status --porcelainis empty.Changeset
.changeset/sqlite-rebuild-applied-honesty.md, graded patch for@objectstack/driver-sql. Justification: it is a reporting-honesty bug fix with no new capability and no removal. The one contract widening —rebuildSqliteTablePatchednow resolving to the honouredManagedDriftEntry[]where it used to resolve to nothing — cannot break a caller (await-and-discard still compiles; the in-repo direct call insql-driver-unique-tenancy.test.ts:451is exactly that shape and is unchanged), only an out-of-tree subclass that overrides aprotectedinternal, which is not a published surface. No workspace subclass overrides it.Generated by Claude Code