fix(engine): keep operator stops clean and never re-execute direct DDL on resume - #898
Draft
aparajon wants to merge 2 commits into
Draft
fix(engine): keep operator stops clean and never re-execute direct DDL on resume#898aparajon wants to merge 2 commits into
aparajon wants to merge 2 commits into
Conversation
…L on resume An operator stop that landed during ALTER routing or direct-execution setup overwrote the stopped state with a spurious failure, and a stop during the size gate was misreported as an unmeasurable table. Stops now stay cleanly stopped on every direct-execution path, and a cancelled size gate is an error distinct from the fail-closed blocked verdict. Resume, volume, and retry no longer re-execute direct statements: each statement's recorded lifecycle is consulted, completed statements are skipped, and an interrupted statement whose server-side outcome is unknown fails the resume closed with re-plan guidance instead of risking silent duplication of non-revertible DDL. Progress entries are reused across resumes so a statement never appears twice. The apply-time size gate and direct executor now connect with the verbatim credentials DSN Plan uses, so DSN parameters like TLS survive to apply time, and the GTID probe reuses the shared DSN builder. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens Spirit’s direct-execution path so operator stop/resume behavior is consistent with engine-driven execution, and ensures non-revertible native DDL is never re-executed on resume by consulting per-statement lifecycle state.
Changes:
- Preserve clean
Stoppedstate across all direct-execution seams (routing, target connect, dedicated-conn acquisition, session lock-wait setup, and size gate cancellation). - Add per-direct-statement “claim” logic so resumes skip already-completed statements and fail closed on unknown outcomes (preventing unsafe re-execution).
- Extend observability/docs and add integration tests covering stop/resume edge cases and per-statement lifecycle tracking.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/metrics/metrics.go | Adds new direct-execution metric outcomes for resume skip and fail-closed unknown-outcome cases. |
| pkg/engine/spirit/spirit.go | Threads verbatim credentials DSN into apply execution; updates plan-time refused-statement verdict handling to propagate cancellation distinctly. |
| pkg/engine/spirit/execution.go | Propagates DSN into execution/resume paths and ensures stop handling during ALTER routing is treated as an operator interruption, not a failure. |
| pkg/engine/spirit/direct.go | Implements cancellation-aware size gate, direct-execution stop checks at multiple seams, and per-statement lifecycle “claim” logic to prevent double execution on resume. |
| pkg/engine/spirit/control.go | Ensures resume/start forwards stored DSN into the resume path. |
| pkg/engine/spirit/spirit_integration_test.go | Updates test call sites for the new execute signature including DSN. |
| pkg/engine/spirit/pending_drops_integration_test.go | Updates test call sites for the new execute signature including DSN. |
| pkg/engine/spirit/direct_integration_test.go | Adds/updates integration tests for stop/resume behavior and per-statement direct lifecycle handling. |
| docs/direct-execution.md | Documents routing/execution order, stop/resume semantics, and expanded metric outcomes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…hase 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.
Why this matters
Direct execution runs non-revertible native DDL, so the operator-control seams around it have to be exact. Two were not.
First, stops didn't stay stopped. An operator stop that landed during ALTER routing or executor setup overwrote the clean stopped state with a spurious failure — the schema change became unresumable. Worse, a stop that landed during the size gate (the row-count check that decides whether a table is small enough to run natively) was misreported as "row count unavailable" and miscounted under
blocked_size_unknown— the exact signal the runbook maps to connectivity problems.Second, nothing consulted the record of completed direct statements. A resume — or a
volumechange, which is internally a stop followed by a start — re-ran the whole plan from the start and re-executed direct DDL that had already landed. In the worst case this was silent: an unnamedFOREIGN KEYre-runs successfully as a second identical constraint, diverging the target from the declared schema under a passing apply.What it does
skipped_completedoutcome); a failed statement re-runs (MySQL DDL is atomic — it left no effect); a statement an earlier run left mid-flight fails the resume closed (blocked_outcome_unknown) with guidance to inspect the table and re-plan, because MySQL may have finished the DDL server-side after the connection dropped. Progress entries are reused across resumes, so a statement never renders twice.Northstar: every operator control operation behaves identically whether it lands during an online copy or a direct statement — stops are always clean, and non-revertible DDL never runs twice.
🤖 Generated with Claude Code