fix(engine): report a drained schema change's terminal outcome instead of pending

[aparajon]

Why this matters

The Spirit engine's Drain() clears a finished schema change and forgets its outcome entirely, so a progress poll arriving right after the drain gets back "pending — no active schema change" even though the change just completed or failed. The driver last recorded the task as running, treats "pending" as no-news, and keeps polling: a silently wedged apply with a healthy lease and no errors anywhere — and the window is easy to hit on multi-operation deployments, where a sibling operation's drive drains every engine on the instance before dispatching its own task. This PR makes the drained outcome survive until the next poll reads it, so finishing work is never erased between completion and observation.

What it does

before: change completes
          └─ sibling drive calls Drain()
               └─ outcome erased from the engine
                    └─ Progress() reports "pending" → driver polls forever

after:  change completes
          └─ sibling drive calls Drain()
               └─ outcome retained: state, message, error text,
                  last per-table progress snapshot
                    └─ Progress() reports the terminal outcome
                         → driver finishes the task

• Drain() retains the final outcome of the change it clears when that change finished as completed or failed. Stopped changes are not retained, because they resume from their checkpoint rather than reporting a terminal result, and cancelled changes resolve through the cancel call itself.
• Progress() serves the retained outcome instead of falling back to "pending", so a poll after the drain sees the truth.
• Stop() and Cancel() answer from the retained outcome too: a control call arriving after the drain gets the same typed already-completed rejection it would have received racing a tracked completion, so the caller reconciles to the completed outcome instead of being told nothing ever ran.
• Apply() clears the retained outcome when it accepts new work, so one change's result never bleeds into the next change's progress.
• Drain()'s blocking behavior — wait for goroutines to exit and release connections — is unchanged.

Two safety properties worth calling out:

• Failed outcomes stay retryable, exactly as a live failure would be — retention changes when the outcome is visible, never what it means.
• A Drain() that lost a race to a newer Apply() leaves the newer change untouched, using the same guard Cancel() already uses.

How it moves us toward the northstar

Engine progress is a display feed; outcomes belong to durable state. Today the drained outcome's only carrier is engine memory, and this PR stops the engine's own cleanup from destroying it before the driver records it durably. It narrows the window the drive loops are moving to eliminate entirely: an outcome should be written down the moment it exists, not held in memory waiting for a poll.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

Fixes a Spirit-engine edge case where Drain() could erase the terminal outcome of a just-finished schema change, causing subsequent Progress() polls to incorrectly return “pending” and potentially wedge the driver’s polling loop.

Changes:

• Retain the terminal outcome (completed/failed) across Drain() via an engine-level drainedOutcome snapshot, and serve it from Progress() when no schema change is currently tracked.
• Clear any retained drained outcome when Apply() accepts new work to prevent cross-run bleed-through.
• Add unit + integration coverage for drained completed/failed outcomes and for “fresh progress” after a drained failure.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
pkg/engine/spirit/spirit.go	Adds drained-outcome retention logic to Drain()/Progress(), clears it in Apply(), and extracts direct-statement progress rendering into a helper.
pkg/engine/spirit/drain_outcome_test.go	Unit tests for drained completed/failed outcomes, idle drain behavior, and non-retention of stopped changes.
pkg/engine/spirit/drain_outcome_integration_test.go	Integration tests exercising drained terminal outcomes through real Apply()/Progress() flows and ensuring drained failures don’t bleed into subsequent applies.

---

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

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1114, 3a742ab.

Verdict: 8 findings — 0 blocking, 2 non-blocking (an unproven safety line, an identity-less outcome that now outlives the drain), 6 suggestions.

Non-blocking

1. Apply's release of the retained outcome is the load-bearing line of the whole design, and no test proves it — the integration test comment claims coverage it structurally cannot provide.
pkg/engine/spirit/spirit.go:789

	// Accepting new work releases the previous change's drained outcome, so
	// one schema change's result never bleeds into the next one's progress.
	e.drainedOutcome = nil

I mutated the PR head by deleting that one line and ran the package under go test -overlay: the full unit suite passed. As a control I deleted if e.runningSchemaChange != rm from Drain in the same way and the suite failed (--- FAIL: TestDrainLosingRaceToNewerChangeLeavesItUntouched, expected: &runningSchemaChange{…} actual: nil), so the harness does reach this code — the survival is a real coverage hole, not a harness artifact.

It is structural, not accidental: Progress consults e.drainedOutcome only inside the e.runningSchemaChange == nil branch (spirit.go:820-823), and Apply installs the new tracked change in the same critical section, so no poll taken after an Apply can ever observe a stale outcome. That makes the comment at drain_outcome_integration_test.go:155-156 — "a poll that resurfaced the first change's failure fails here" — an assertion about a failure mode that test cannot experience.

The sequence the line actually protects is one nothing covers: A completes → Drain retains completed → Apply B → B is stopped by an operator → Drain (stopped is not retained, and Drain does not clear an existing outcome, spirit.go:342-345) → the next Progress serves A's completed for B's stopped, half-copied work. A four-line unit test (apply-after-drain, then drain a stopped change, assert pending) kills the mutant. AGENTS.md:116 — "Tests must prove documented behavior" — and the PR body documents exactly this behavior.

2. The retained outcome carries no apply/task identity and now survives past the drain, so the stale-task conflict check can stamp a different apply's resting task terminal.
pkg/engine/spirit/spirit.go:823 / pkg/tern/local_apply.go:376

tryResolveStaleTask protects resting (stopped / failed-retryable) tasks by keyword on the engine's message:

	if result.Message == "No active schema change" {
		if !state.IsInFlightTaskState(t.State) {

A retained outcome displaces that message with "Schema change completed", and result.State.IsTerminal() at local_apply.go:347 is checked first, so B's stopped task is transitioned to completed — a schema change recorded as applied that never ran. Spirit ignores the ResumeState.MigrationContext the drive stamps on the probe (local_apply.go:334), so the answer is engine-wide, not task-scoped. The remaining guards are weak for this case: apply.LeaseOwner != "" && !LeaseOwnedByThisProcess (:353) passes when the same pod drove both applies.

Marked non-blocking, not blocking, because I could only partly confirm it: the hazard is pre-existing, and the PR widens rather than creates it. Pre-PR, runningSchemaChange is cleared in exactly two places — spirit.go:345 and control.go:147 — so a completed change already answers unrelated probes as completed until something drains it. What changes here is that Drain was one of the two things that reset the answer to the sentinel, and the two drive paths that drain without a following Apply (local_control_resume.go:351, :725) now leave the terminal answer installed indefinitely. The durable fix is the one pkg/engine/postgres already uses — key the retained outcome on ResumeState.MigrationContext so an unrecognized identity reads the idle sentinel. Fine as a follow-up.

General suggestions

3. Apply's zero-DDL early return accepts the request before both the Drain and the clear, contradicting the new field's documented invariant.
pkg/engine/spirit/spirit.go:721

	if len(req.FlatDDL()) == 0 {
		return &engine.ApplyResult{
			Accepted: true,

The field comment says "Apply releases it when it accepts new work" (:86-88), but this accepted path returns before e.Drain() at :744 and before the clear at :789, so an accept-then-poll caller would read the previous change's outcome as its own. I traced the drive paths and could not reach it (replanAndFilterTasks / tableStillNeedsChange filter empty work; EnsureSchema returns at ensure_schema.go:246 when nothing is allowed), so this is an invariant/robustness note only — moving the clear above the early return closes it.

4. A drained completed outcome forces Progress = 100 while keeping the mid-copy RowsTotal, the exact shape the live path documents that it reconciles away.
pkg/engine/spirit/spirit.go:388

			if rm.state == engine.StateCompleted {
				tp.Progress = 100

buildSpiritTableProgress does tp.RowsTotal = tp.RowsCopied when a table is complete (:968-970), and its doc explains why: "a full bar contradicted by its own rows line" (:947-951). The drained branch produces 49000/50000 at 100%, pinned by drain_outcome_test.go:154-156. I checked the display claim both finders made and it does not hold — the read path clamps for completed tasks (if tp.RowsCopied < tp.RowsTotal { tp.RowsCopied = tp.RowsTotal }, local_client.go:2929-2933) — so this is an engine-contract consistency point, not a rendering bug.

5. Only Progress learned about the retained outcome; Cancel still answers "no active schema change" for the same drained completion.
pkg/engine/spirit/control.go:113

		return nil, engine.NewPermanentError("no active schema change to cancel")

While the change is still tracked, Cancel returns the typed AlreadyCompletedError that lets the drive settle the apply (control.go:115-121, consumed at local_control.go:656); after a drain that retained completed, Progress says completed and Cancel says nothing ever ran. The Cancel behavior is unchanged by this PR, but this is where the information needed to answer correctly starts being kept, so serving it from Cancel/Stop is the natural completion of the fix.

6. retainsDrainedOutcome's cancelled clause is documented but untested.
pkg/engine/spirit/spirit.go:366

func retainsDrainedOutcome(s engine.State) bool {
	return s == engine.StateCompleted || s == engine.StateFailed
}

The doc says "cancelled changes resolve through the cancel call itself, so neither is retained", but only StateStopped has a test (TestDrainDoesNotRetainStoppedChange). A later widening to s.IsTerminal() would silently start retaining cancelled changes with nothing failing. A sibling test with rm.state = engine.StateCancelled is two lines.

7. newDrainedOutcome's no-live-poll fallback re-implements Progress's fallback loop.
pkg/engine/spirit/spirit.go:398 vs :871

Both walk rm.tables and index-align rm.ddls[i] to build the same identity/DDL entries. The PR already extracted directStatementTableProgress for exactly this reason; the same treatment here keeps the two index-alignment assumptions from drifting apart.

8. drainRaceWindow is read under e.mu but written without it.
pkg/engine/spirit/spirit.go:319

raceWindow := e.drainRaceWindow is snapshotted inside e.mu.Lock(), while the only writer sets it unsynchronised (drain_outcome_test.go:188). No race today — the seam is installed before any drain, on the same goroutine, and go test -race ./pkg/engine/spirit/ is clean — but a seam whose whole purpose is interleaving concurrent activity invites a future test that installs it mid-drain. Setting it under e.mu in the test costs nothing.

The one thing that could have broken, verified

The riskiest mechanism is the new lifetime of engine state: the release point for a terminal answer moved from Drain to Apply, on an engine that is shared per-database for the process lifetime — and the retained answer carries no apply identity, so any consumer polling in the widened window adopts a foreign terminal state.

I bounded it in four ways and it holds inside the package, but not entirely outside it. (1) Progress serves the outcome only under e.runningSchemaChange == nil (spirit.go:820), so any tracked change shadows it. (2) Apply installs the new change and clears the outcome under one lock acquisition (:786-789), so no interleaving exposes new-change-plus-old-outcome. (3) The operator read path renders from storage and never polls the engine (local_client.go:2902-2934), and the terminal-truth reconciler excludes instance-local engines via ProgressIsExternallyAuthoritative, which only planetscale implements — so neither can adopt a retained outcome. (4) The drive poll loops always poll after their own Apply, which clears.

The one ingress that survives all four is tryResolveStaleTask, whose resting-task protection is keyed on the literal sentinel message the retained outcome now displaces — finding 2. I confirmed via the merge base that the hazard predates this PR (a completed change stays tracked and reports completed until something clears it) and that Drain was merely one of the incidental things that used to reset it, which is why I filed it non-blocking rather than blocking.

Verified correct

• The new ownership guard is a real bug fix, not a hypothetical: the merge base's Drain ran e.runningSchemaChange = nil unconditionally after rm.wg.Wait() (git show 58336d1:pkg/engine/spirit/spirit.go, lines 305-307), so a drain that lost the race erased a newer change's tracked state. Deleting the new guard at spirit.go:333 under -overlay fails TestDrainLosingRaceToNewerChangeLeavesItUntouched with actual: nil — the test genuinely pins it. The guard mirrors Cancel's at control.go:146.
• The losing-race branch unlocks e.mu before logging (spirit.go:337-339), and schemaChangeLogger takes no lock (:269-274); the seam runs outside the lock too, so a seam that calls back into the engine cannot deadlock.
• The extracted directStatementTableProgress (:916-938) is field-for-field the loop deleted from Progress, including the copy-then-address StartedAt/CompletedAt pattern and the directStateCompleted → Progress = 100 branch; with no direct statements it returns an empty slice, so Tables nil-ness in ProgressResult is unchanged.
• rm.lastLiveTables = slices.Clone(tableProgress) at :868 is assigned before the direct-statement append at :890, so newDrainedOutcome's own append at :413 cannot double-count a direct statement.
• Retryable semantics are identical between live and drained: Retryable: d.state == engine.StateFailed (:828) matches Retryable: state == engine.StateFailed (:904).
• progressState only refines non-terminal states (:1042-1050), and only completed/failed are retained, so the drained outcome's raw rm.state is exactly what a live poll would have reported — no waiting-for-cutover or volume-restart state can be flattened into a terminal answer by the drain.
• retainsDrainedOutcome covers the terminal states this engine can actually reach: completed/failed are set by the execution goroutine (execution.go:437-455), stopped and cancelled by control.go:81 and :128.
• go test -count=1 -race ./pkg/engine/spirit/ passes on the head commit, and all 34 CI checks are green (Unit, Integration per-package, E2E MySQL/Vitess/K8s, all four lint jobs).

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

[aparajon]

🤖 Thanks — addressed at e0b6d7b, finding by finding:

1. Fixed. The release point is now a named seam: installRunningSchemaChange installs the tracked change and clears the retained outcome in one critical section, the production Apply and the test harness's registerRunningSchemaChange both route through it, and TestAcceptingNewWorkReleasesDrainedOutcome runs your exact sequence — A completes, drain retains it, B is installed, B is stopped, drain again, and the poll must read pending, not A's completion. Your overlay mutant (deleting the clear) now fails that test. The integration-test comment that claimed resurfacing coverage it structurally couldn't provide is corrected to describe what the test does prove.
2. Follow-up. Agreed on both the hazard and the durable fix — keying the retained outcome on ResumeState.MigrationContext the way pkg/engine/postgres does, so an unrecognized identity reads the idle sentinel while identity-less probes keep the drained answer. That's queued as its own change; it touches the probe contract in tryResolveStaleTask and deserves its own tests rather than riding along here.
3. Fixed. The zero-DDL accept now clears the retained outcome before returning, so the field's "Apply releases it when it accepts new work" invariant holds on every accepted path, reachable or not. TestApplyWithNoChangesReleasesDrainedOutcome pins it (the credentials and zero-DDL checks both precede any dial, so the test runs without a target).
4. Fixed. The drained completed branch reconciles RowsTotal = RowsCopied, matching buildSpiritTableProgress's treatment of the same shape — the completed count is ground truth, the total was an estimate. The pinned test now expects 49000/49000, so the engine contract no longer relies on the read path's clamp.
5. Done, for both Stop and Cancel. When the nil-tracked-change branch finds a drained completed outcome, both now return the typed AlreadyCompletedError (naming the database) that the drive already consumes on each path, so a control call landing after the drain settles the apply instead of being told nothing ever ran. A drained failed outcome deliberately stays a PermanentError — there is no completion to reconcile to — and TestStopAndCancelAfterDrainedFailureStayPermanent pins that boundary alongside the two already-completed tests.
6. Fixed. TestDrainDoesNotRetainCancelledChange is the cancelled sibling; a widening of retainsDrainedOutcome to s.IsTerminal() now fails it (and the stopped test).
7. Fixed. Extracted tableIdentityProgress; newDrainedOutcome's fallback and Progress's fallback both build their identity/DDL entries through it, so the index-alignment assumption lives in one place.
8. Fixed. The test installs the seam under e.mu.

Every fix was mutation-verified: reverting each one (dropping the clear in installRunningSchemaChange, dropping the zero-DDL clear, dropping the rows reconcile, removing both drained-completed control branches, widening retainsDrainedOutcome) fails at least one test. go build, go vet -tags=integration, and go test -race ./pkg/engine/... ./pkg/tern/... are green on e0b6d7b.

This reply was generated by Claude Code (Claude Fable 5).