fix(move): make --force re-validate before wiping and cover definitive unresumable checkpoints#1038
Draft
morgo wants to merge 1 commit into
Draft
fix(move): make --force re-validate before wiping and cover definitive unresumable checkpoints#1038morgo wants to merge 1 commit into
morgo wants to merge 1 commit into
Conversation
…e unresumable checkpoints --force's wipe-and-restart recovery had two holes: 1. It wiped the target FIRST and re-ran the post-setup checks AFTER. RunChecks iterates a map, so the triggering failure may equally have been source-side (rename_safety's leftover _old table, source_schema_consistency drift, table_compatibility) — failures that wiping the target cannot cure. A multi-hundred-GB partial copy plus a valid checkpoint could be destroyed only for the rerun to fail on the same source-side error. Now the post-setup checks minus target_state (the one check wiping cures) are re-run BEFORE the wipe, and the wipe is refused if any still fail. 2. When the resume probe passed but resumeFromCheckpoint's deeper validations found the checkpoint definitively unusable — older than --checkpoint-max-age, positions that do not parse (e.g. a row written by the migration runner into the same _spirit_checkpoint table name), or a source missing from the positions map — the run hard-failed even with --force set, despite the flag's contract covering exactly "cannot resume from a checkpoint". These states are now tagged with typed sentinels (status.ErrCheckpointTooOld, new errCheckpointUnresumable) and fall through to the same validate-then-wipe path. Possibly-transient errors keep hard-failing: classification is strictly by errors.Is, never by string. resumeFromCheckpoint now performs all checkpoint-record validation before its first side effect (chunker subscriptions, runner state), so a definitive validation failure leaves the runner clean for the fall-through to rebuild everything via newCopy(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
--force's wipe-and-restart recovery inpkg/movehad two holes, both in the post-setup decision block insetup():It wiped the target before checking whether the wipe could help. On a
ScopePostSetupfailure with resume not possible,--forceranwipeTargets(dropping every target table copy and the checkpoint) first, and re-ran the post-setup checks after.RunChecksiterates a map, so the triggering failure may equally have been source-side —rename_safety's leftover<table>_oldon a source,source_schema_consistencydrift on one shard,table_compatibility(no PK) — none of which wiping the target can cure. A multi-hundred-GB partial copy plus a valid checkpoint could be destroyed only for the rerun to fail on the very same source-side error.--forcesilently could not recover the failures its probe doesn't cover.canResumeFromCheckpointonly runs the resume pre-checks and verifies the checkpoint row is readable. If it said "resumable",resumeFromCheckpoint's deeper validations — checkpoint older than--checkpoint-max-age, a positions payload that doesn't parse (e.g. a row written by the migration runner into the same_spirit_checkpointtable name stores a single opaque position, not move's JSON map), or a source missing from the positions map — returned a hard"resume validation passed but checkpoint resume failed"error with no--forcefallback, even though theForcedocstring promises wipe-and-restart for exactly "cannot resume from a checkpoint" states. The operator had toDROPthe target tables by hand.Fix
Curable vs. non-curable classification. Wiping the target cures exactly one post-setup check:
target_state(non-empty/mismatched target tables; the wipe also removes the checkpoint). Everything else registered atScopePostSetupis source-side. The check framework gainscheck.RunChecksExcluding(ctx, r, logger, scope, exclude...)plus an exportedcheck.TargetStateCheckNameconstant, so the classification is by registered check name — no error-string matching anywhere. New post-setup checks are deliberately included in the pre-wipe validation by default: excluding too little only blocks a wipe, excluding too much could green-light one.Ordering (bug 1). After deciding "not resumable +
--force", the runner now re-runs all post-setup checks excepttarget_statebefore wiping. If any fail, it returns"force: refusing to wipe the target because a check that wiping cannot fix is failing: …"with the target intact. Only then does it wipe and re-run the full check set as before (which now also validatestarget_stateagainst the wiped target). Contract:--forcenever destroys target data when the run would still fail for source-side reasons.Definitive unresumable states (bug 2).
resumeFromCheckpoint's record validations are now typed:status.ErrCheckpointTooOld(unchanged; its message now also mentions--force),errCheckpointUnresumablesentinel.When resume fails and
--forceis set,isDefinitivelyUnresumable(err)(strictlyerrors.Isagainst those two sentinels) decides whether to fall through to the same validate-then-wipe path. Anything else — connection failures, lock timeouts — still hard-fails, so a maybe-transient blip can never trigger a wipe. Without--force, all of these remain the same hard errors as before (including the specific too-old guidance).To make the fall-through safe,
resumeFromCheckpointnow performs the whole record validation (read, age, positions parse, per-source key presence) before its first side effect (chunker subscriptions on the repl clients, runner watermark state). A definitive validation failure therefore leaves the runner exactly as it found it, and the fall-through rebuilds everything vianewCopy()— no double subscriptions, no leaked checksum watermark.Testing
New regression tests (all verified to fail against the pre-fix implementation — the Fix-A pre-fix failure literally reads
"force: target still fails post-setup checks after wiping: table 't1_old' already exists on source 0", i.e. the data was already gone):TestMoveForcePreservesTargetOnSourceSideFailure: target holds a partial copy + unresumable checkpoint; source has a leftovert1_old.--forcemust fail with "refusing to wipe", leaving the target rows and checkpoint untouched; after droppingt1_old, the same--forcemove wipes and completes.TestMoveForceRecoversUnparseableCheckpointPositions/TestMoveForceRecoversCheckpointMissingSource/TestMoveForceRecoversTooOldCheckpoint(shared helper): produce a real checkpoint viacheckpointAndStop, corrupt it (binlog_position = '<opaque>','{}', or backdatecreated_at8 days), then assert both sides: without--forcea hard failure with the right sentinel (errors.Is) and an untouched target; with--forcea completed fresh copy (usedResumeFromCheckpointstays false).Existing coverage re-run green:
TestMoveForceWipesUnresumableTarget,TestResumeFromCheckpointTooOld/NotTooOld,TestResumeFromCheckpointE2E/MultiTableE2E/CompositePKE2E,TestMultiSourceResumeFromCheckpointE2E,TestMultiSourceResumeDiscardsChecksumWatermark,TestSingleSourceResumeKeepsChecksumWatermark,TestMoveResumeDeletesAboveWatermark,TestBasicMove,TestEmptyDatabaseMove, plus./pkg/move/check.gofmt,go build ./...,go vet,golangci-lint run ./pkg/move/...all clean.Note: open PR #1029 splits
setup()intosetupDiscovery/setupUnderLocks. This PR is based onmainand keeps its changes inside the force/resume decision block,resumeFromCheckpoint, and helpers (noRun()ordering changes), so whichever lands second needs a small mechanical rebase of that block.🤖 Generated with Claude Code