Skip to content

fix(schema): survive dolt#11131 encoding drift in the aux row re-key (#4380) - #5064

Open
maphew wants to merge 1 commit into
gastownhall:mainfrom
maphew:fix/aux-rekey-encoding-drift
Open

fix(schema): survive dolt#11131 encoding drift in the aux row re-key (#4380)#5064
maphew wants to merge 1 commit into
gastownhall:mainfrom
maphew:fix/aux-rekey-encoding-drift

Conversation

@maphew

@maphew maphew commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Why

Fixes #4380.

A database whose events / comments storage carries dolthub/dolt#11131 encoding drift cannot be opened by bd 1.1.0 at all. The aux row-id re-key scans every column of the four aux tables; on a drifted table that scan panics inside Dolt (invalid hash length: 19), which aborted the whole MigrateUp pass — and since the pass is re-attempted on every open, the panic recurred on every start. The only build that could read the data was the pre-re-key one, so affected users had no upgrade path onto 1.1.0, forward or back.

Five independent environments confirmed it on #4380, four of them on released Homebrew bd 1.1.0 + dolt 2.1.10, including single-user remote-backed databases — this is not a fleet-only or shared-server-only failure. @aaronlippold summarised it as "a breaking change for upgrade from 1.0.5 to 1.1.0".

The drift itself is not bd's to repair: the rows are physically undecodable, no SQL read or write can touch them, and healing them needs Dolt's schema-encoding-drift recover-rows (dolthub/dolt#11133), which @marcodelpin has since verified end-to-end on a live database. Dolt 2.1.10 stops new drift, and bd's own migration 0057 already guards the 0048 re-application that produced it locally. What was still missing is that the migration path has to survive tables it cannot read. That is all this PR does.

Credit

The fix is @marcodelpin's, from 767d5dcd on their fork, absorbed here with the semantic rework requested in my review on #4380 rather than leaving them to do another round trip. The diagnosis behind it — the chunk-level characterisation, the negative result on the minimised fixture, the dolt 2.1.10 re-test, the recover-rows verification — is theirs too, on the issue. isSchemaEncodingDriftErr and its table-driven test come from that commit.

What changed vs. the fork commit

The fork's skip cleared the in-progress sentinel and returned nil, so ignoredSource.migrate recorded marker version 9 later in the same pass and !markerPending then barred re-entry forever. The warning promised "re-run the re-key after the storage drift is repaired", but no path existed to do so. On a single shared sql-server that loss is moot (one copy of every row, clones never merge), as the commit message says; upstream serves merge topologies, where the skipped table would keep its per-clone-random 0037 ids permanently.

So the skip is now recorded and resumable:

  • New clone-local record aux_row_rekey_drifted in local_metadata (dolt-ignored, like the sentinel), naming the skipped tables. It re-admits the re-key after the marker is recorded, and a post-marker pass re-keys only the recorded tables. The others converged in the completed pass, and rows minted since carry app-minted ids already consistent across clones — re-keying those on one clone alone would manufacture the divergence the re-key exists to remove. Scoping keys on the marker, not on the sentinel, so a retry that itself dies on some unrelated error stays scoped rather than widening back to all four tables on the next pass.
  • The in-progress sentinel keeps its exact meaning — "a rewrite is in flight" — and is cleared on a skip like any completed pass. It is deliberately not reused as the durable record: MigrateUp reads it to exempt the aux tables from the pre-existing-dirty and changed-signature guards, which is right for a crashed pass's partial UPDATEs sitting in the working set, but leaving it set indefinitely would relax those guards for the life of the clone.
  • MigrateUp extends that exemption to the recorded drifted tables, and it matters more there than for the crash case: the changed-signature guard reads dirty tables through dolt_diff, which decodes exactly the cells that panic, so a drifted table left in dirtyBefore would fail the pass on the read and re-create the unopenable database this is meant to end.
  • Both records are read in one round trip (readAuxRekeyState), so MigrateUp and the re-key each cost one existence probe instead of two apiece.

TestRekeyAuxRowIDsKeepsSentinelOnFailure's invariant is untouched: a non-drift failure still aborts and still keeps the sentinel.

Things worth knowing before you merge

  • A resumed pass re-keys the whole drifted table, including rows minted between the skip and the repair, exactly as the existing crash-resume path does. That is why the retry rides every subsequent migration pass instead of waiting to be asked — the sooner it lands, the smaller that set.
  • The retry needs a pass to run. MigrateUp short-circuits on migrationWorkNeeded, so an affected clone in steady state retries when the next migration ships, not on every open. Deliberate: the alternative is re-scanning a large events table on every command for a repair that may be months away. An explicit trigger (bd doctor surface, or a flag on bd migrate) is the obvious follow-up and I'm happy to take direction on it.
  • local_metadata is ephemeral — migration 0030 says so explicitly: recreated empty on a working-set reset. A reset therefore loses the drift record and makes the skip permanent and silent. The in-flight sentinel has carried that same fragility since bd-578h9.16; flagging it rather than pretending otherwise.
  • Drift also produces readable garbage (~10% of affected cells in @marcodelpin's characterisation return a zero payload). Those scan without error, so their rows are re-keyed from corrupt content and converge to an id no healthy clone derives. Nothing here can detect that; it is one more reason the storage repair is still required. The code comment says so.
  • Warnings go to the standard logger, not the package's stderr progress writer, which is TTY-gated to io.Discard. That writer is right for progress chatter; a silent skip for piped/CI/agent callers — the ones least likely to notice divergent ids later — is not. This reverses the mechanical suggestion in my own review, for that reason.

On the classifier

Kept as the bare invalid hash length substring rather than tightened to panic recovered: invalid hash length or to : 19, also reversing an optional suggestion in my review, because both narrower forms have a worse failure mode here — a miss falls through to the abort that leaves the database unopenable:

  • The width varies with the direction of the tag flip. Migration 0057's own comment records both invalid hash length: 19 on read and invalid hash length: 1 on insert/merge for this same drift.
  • The panic recovered: wrapper is added by the SQL engine; a path surfacing the panic unwrapped would not match.

The cost is bounded: dolt raises the phrase from hash.New generally, but reaching this classifier at all means the re-key could not read the table, so skipping and recording it is the same correct answer. Everything without the phrase still aborts the pass.

On the storage boundary

Classifying a Dolt panic message inside internal/storage/schema brushes against the driver direction in docs/PROJECT_CHARTER.md. This package already hosts Dolt-message classifiers in the same style (RemoteRefUnavailableErr, MissingMigrationObjectErr), and this is migration survival for rows that are unreadable by design, so it seems acceptable here — flagging it so the storage-layer owners can say whether drift detection should eventually move behind the driver interface. @coffeegoddd.

Testing

  • go test ./internal/storage/schema/ and ./internal/storage/... green; go vet and gofmt clean.
  • New in aux_row_id_drift_test.go: the classifier table (from the fork, plus the : 1 and unwrapped-panic cases); a drifted table skipped while the pass completes and the record is written; drift surfacing mid-rewrite rather than on the scan, asserting the half-re-keyed table is recorded and committed; a non-drift error still aborting; a post-marker pass touching only the recorded table; the same when the sentinel is also set from a crashed retry; and the record surviving a still-unrepaired retry. sqlmock matches in order, so "exactly one table probe" is a real assertion that the other three are untouched.
  • Two existing tests updated for the changed probe shape, not for changed behaviour.

Not exercised end-to-end against real drifted storage — the panic cannot be produced by any plain Dolt operation (see dolthub/dolt#11131), and @marcodelpin found their minimised fixture still carried recoverable fragments of production content, so it could not be shared. Their standing offer to run diagnostics by proxy against the drifted database is the way to close that gap; I'd like to take them up on it for this branch before it lands.

…astownhall#4380)

A database whose events/comments storage carries dolthub/dolt#11131 encoding
drift could not be opened by bd 1.1.0 at all. The aux row-id re-key scans every
column of the four aux tables; on a drifted table that scan panics inside Dolt
("invalid hash length: 19"), which aborted the whole MigrateUp pass — and since
the pass is re-attempted on every open, the panic recurred on every start. Only
the pre-re-key build could read the data, so affected users had no upgrade path
onto 1.1.0 in either direction.

The drift is not bd's to repair: the rows are physically undecodable and no SQL
read or write can touch them. Dolt 2.1.10 stops new drift and bd's own migration
0057 guards the 0048 re-application that produced it locally; what was missing is
that the migration path has to survive tables it cannot read.

The re-key now skips such a table, records it, and completes the pass:

- isSchemaEncodingDriftErr classifies the panic signature. Deliberately the bare
  "invalid hash length" phrase: 0057 documents both ": 19" on read and ": 1" on
  insert/merge for the two directions of the tag flip, and the "panic recovered"
  wrapper is added by the SQL engine, so both narrower matches risk falling
  through to the abort that leaves the database unopenable. Every error without
  the phrase still aborts the pass.
- A new clone-local record (aux_row_rekey_drifted in local_metadata) names the
  skipped tables and re-admits the re-key, which the marker alone can no longer
  do once the completed pass records it. A post-marker pass re-keys only the
  recorded tables: the others converged, and rows minted since carry app-minted
  ids already consistent across clones, so rewriting them on one clone would
  manufacture the divergence the re-key exists to remove.
- The in-progress sentinel keeps meaning "a rewrite is in flight" and is cleared
  on a skip like any completed pass. MigrateUp reads it to relax its dirty-table
  guards, which is right for a crashed pass's partial UPDATEs but must not become
  permanent; the drift record now carries that exemption instead, for the
  recorded tables only — the changed-signature guard reads dirty tables through
  dolt_diff, which decodes exactly the cells that panic.
- Both records are read in one round trip (readAuxRekeyState), so MigrateUp and
  the re-key each cost one probe rather than four.

The skip warnings go to the standard logger rather than the package's progress
writer, which is TTY-gated to io.Discard: a piped or CI caller would otherwise
have no notice at all that a table's ids stayed divergent.

Not exercised against real drifted storage — the panic cannot be produced by any
plain Dolt operation, and the reporter's minimised fixture could not be shared
because it retained recoverable production content.

Co-authored-by: Marco Del Pin <marco.delpin@gmail.com>
Agent-Signature: claude-opus-5-xhigh on behalf of maphew
@maphew

maphew commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Status note: this fix shipped today as hotfix release v1.1.2, cherry-picked onto a v1.1.0-based branch (hotfix/v1.1.1, commit 564049b) so bricked 1.1.0 users regain an upgrade path — see the announcement on #4380. (The v1.1.1 tag exists but was never published; its release run failed on a stale MCP lockfile and the number was burned rather than rewriting a pushed release tag.)

This PR remains the main-line landing and its gate is unchanged: it still wants storage-owner input on the classifier-in-schema-package question, and reporter validation against real drifted storage — which the shipped release should now make possible.

claude-fable-5-xhigh on behalf of maphew

maphew added a commit that referenced this pull request Jul 26, 2026
v1.1.2 shipped 2026-07-26 off the v1.1.0 tag (branch hotfix/v1.1.1)
carrying the dolt#11131 aux-rekey encoding-drift fix (#4380). This
back-merges the release metadata main still needs: the [1.1.2]
CHANGELOG section (which also explains the burned 1.1.1 number) and
the cmd/bd/info.go versionChanges entry.

Deliberately NOT carried from the hotfix branch: the fix itself
(reaches main via #5064, still review-gated), the version bumps
(release-branch concerns), and the Docusaurus versioned-docs
snapshot (main deleted the Docusaurus site in 52aea3f for
Mintlify).

Agent-Signature: claude-fable-5-medium on behalf of matt wilkie

Claude-Session: https://claude.ai/code/session_01AQHiBWVPSXDCcUSG9EGw9d

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@maphew maphew added the triaged Issue has been reviewed and responded to label Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triaged Issue has been reviewed and responded to

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Aux row re-key crashes the migration pass on dolt#11131-class drifted storage (server panic 'invalid hash length: 19') — fix: skip-and-warn per table

1 participant