Skip to content

move: rename --create-sentinel to --defer-cutover - #1158

Merged
morgo merged 2 commits into
block:mainfrom
morgo:unify-defer-cutover-flag
Aug 19, 2026
Merged

move: rename --create-sentinel to --defer-cutover#1158
morgo merged 2 commits into
block:mainfrom
morgo:unify-defer-cutover-flag

Conversation

@morgo

@morgo morgo commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Move's --create-sentinel does the same thing as migrate's --defer-cutover: it creates _spirit_sentinel and blocks before cutover until an operator drops it. Two names for one concept is needless friction for anyone who uses both commands, so this unifies on the migrate name.

The Go field Move.CreateSentinel (and check.Resources.CreateSentinel) is renamed to DeferCutOver to match migration.Migration.DeferCutOver.

No backward-compatible alias is kept — --create-sentinel is now an unknown-flag error.

Changes

  • pkg/move/move.go — flag and field rename. Help text reworded to match migrate's framing: "Defer cutover (and continuous checksum) until the sentinel table on the first target database is dropped".
  • pkg/move/check/check.goResources.DeferCutOver.
  • pkg/move/runner.go — call sites and a comment.
  • docs/move.md — section heading, TOC entry and anchor, prose. Alphabetical position in the config list is unchanged (checkpoint-max-agedefer-cutoverdefer-secondary-indexes).
  • pkg/move/README.md and the pkg/move tests.

TestCreateSentinelTableIdempotent keeps its name: it tests sentinel.Create idempotency, not the flag.

Testing

go build ./... and go vet ./... clean; golangci-lint run ./pkg/move/... reports 0 issues. TestMoveSentinelDropReleasesCutover and TestCreateSentinelTableIdempotent pass against a local MySQL 9.7. The full pkg/move suite is left to CI.

🤖 Generated with Claude Code

Move's --create-sentinel flag does the same thing as migrate's
--defer-cutover: it creates _spirit_sentinel and blocks before cutover
until an operator drops it. Unify on the migrate name so the two
commands read the same way.

The Go field Move.CreateSentinel (and check.Resources.CreateSentinel)
is renamed to DeferCutOver to match migration.Migration.DeferCutOver.
No backward-compatible alias is kept for the old flag name.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR unifies Move’s sentinel flag naming with Migrate by renaming --create-sentinel to --defer-cutover, and carries that rename through Move’s config struct, runner wiring, documentation, and tests to reduce cross-command friction.

Changes:

  • Renames the Move CLI flag and Go field from create-sentinel/CreateSentinel to defer-cutover/DeferCutOver (no compatibility alias).
  • Updates Move runner call sites and check resources wiring to use the new field name.
  • Updates Move docs and tests to reference defer-cutover.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/move/move.go Renames the Kong flag/field to --defer-cutover / DeferCutOver.
pkg/move/runner.go Updates sentinel creation condition and check resource wiring; updates a related comment.
pkg/move/check/check.go Renames Resources field to DeferCutOver.
docs/move.md Renames the configuration section/anchor and references to defer-cutover.
pkg/move/README.md Updates internal Move package docs to refer to DeferCutOver.
pkg/move/sentinel_test.go Updates test description and Move struct usage to DeferCutOver.
pkg/move/runner_test.go Updates tests to use DeferCutOver and adjusts related comments.
pkg/move/move_test.go Updates tests to use DeferCutOver.
pkg/move/move_sharded_test.go Updates sharded move test to use DeferCutOver.
pkg/move/checksum_invariant_test.go Updates checksum invariant test harness to use DeferCutOver.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread docs/move.md Outdated
Comment thread pkg/move/runner.go
Both from PR review feedback; neither is a logic change.

docs/move.md told operators recovering from a too-old checkpoint to
wipe `_spirit_checkpoint`, but move's checkpoint table is
`_spirit_move_checkpoint`. The distinct name is deliberate (see
pkg/move/README.md) — following the old text would drop the wrong
table and hit the same failure on the next run.

The comment above sentinel.Create claimed "a resume recreates it".
It does not: sentinel.Create is only reached from newCopy. A resume
does not need to recreate it — the sentinel lives on the target and
survives, and the existence-driven sentinel.Wait blocks again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Kiran01bm

Copy link
Copy Markdown
Collaborator

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1158, 10c3afb.

Verdict: 1 finding — non-blocking (pre-existing dead field surfaced by the rename); otherwise clean, approve.

Non-blocking

  • pkg/move/check/check.go:39Resources.DeferCutOver (renamed from CreateSentinel) is written once at runner.go:1601 but read by no check function in pkg/move/check/*.go. Confirmed via history that the field was already write-only before this PR (a pre-existing dead field, not a regression), but the rename makes it a good moment for a follow-up: either wire it into a check or delete it.

General suggestions

  • This PR bundles a docs correctness fix (_spirit_checkpoint_spirit_move_checkpoint in docs/move.md:34) into what is otherwise a pure identifier-rename PR. Verified the fix is correct (see below), but scope-wise a follow-up PR might prefer keeping "rename" and "fix a stale doc fact" as separate, independently revertible changes.

The one thing that could have broken, verified

The riskiest mechanism is whether the resume path still correctly waits-or-doesn't-wait on the sentinel after the rename touched the surrounding comment and field. Traced the dispatch in setupUnderLocks: only resumeFreshOwned/no-checkpoint paths call r.newCopy (which gates sentinel.Create on r.move.DeferCutOver at runner.go:960); a true resumeCheckpoint resume skips it entirely. Both paths converge on the same unconditional sentinel.Wait call at runner.go:1292, whose Exists callback (not the DeferCutOver flag) decides whether to block — exactly matching the new comment's claim that a resume "simply survives" via the existing table, or cuts over immediately if the operator already dropped it. The field itself is unchanged (bool, default:"false") at move.go:23 — only the kong tag name, help text, and comments changed. The rename is mechanical with zero behavior change.

Verified correct

  • Rename sweep is complete: the only surviving CreateSentinel string in pkg/move is the deliberately-unrenamed test name TestCreateSentinelTableIdempotent (it tests sentinel.Create, not this flag).
  • pkg/migration already used DeferCutOver/WithDeferCutOver before this PR (e.g. TestDeferCutOverE2E in cutover_test.go), so the rename correctly brings pkg/move in line with the existing sibling-runner naming.
  • The bundled docs fix is accurate: the real move checkpoint table name is _spirit_move_checkpoint, per the constant at check/resume_state.go:39, matching the corrected doc text.
  • pkg/datasync has no cutover/sentinel concept — its only related bool is Force (drop-and-recreate-on-no-resume, unrelated to cutover gating) at sync.go:85 — so it correctly needed no changes for this rename.

This review was generated by Claude Code (claude-sonnet-5).

@morgo
morgo merged commit e4b69b6 into block:main Aug 19, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants