fix(engine): settle tasks stranded under a terminal apply, and stop counting refused writes as success

[aparajon]

Why this matters

A stopped schema change could end up stuck: it could not be resumed, could not be replaced by a new one, and would not let go of its database. Clearing it took a hand-edited row.

Two things went wrong together. The stop marked the change as stopped, failed to write the state of the tables underneath it, and reported success anyway. That left a table in a state nothing else knew how to deal with.

What it does

           BEFORE                     |            AFTER
--------------------------------------+------------------------------------
 stop:  change -> stopped             |  stop:  change -> stopped
        table  -> write refused,      |         table  -> write refused,
                  counted as success  |                   so the stop fails
                                      |
 the table is now stuck. Nothing      |  a table left under a finished
 resumes it, nothing clears it,       |  change is settled for it, so
 and it blocks the whole database.    |  start can resume the change.

• A stop or cancel no longer claims to have done work it didn't do. SchemaBot holds two separate leases: one on the change, one on the work driving it. A stop can still hold the first after another server has taken the second, and when that happens its table writes are rejected. Those rejections were logged and then counted as successes. They are now counted as failures, and the command returns an error instead of marking the change stopped on top of tables that never moved. Every table is still attempted, so a retry only has the rejected ones left to do.

• Nothing is left waiting for a driver that will never come. Once a change reaches a final state, no server will ever pick its tables up again. Two places already tried to clean this up, but each recognized only one specific table state, so a table in any other state stayed stuck. Both now handle any table sitting under a finished change. A table under a stopped change is settled to stopped rather than cancelled, which is what lets a later start pick its copy back up instead of throwing the work away.

• start can find the change again. It compared states as plain strings, so a state written in its protobuf form didn't match and a change that was ready to resume got refused. It also applied its seven-day search window to changes the operator named explicitly, so an older one couldn't be resumed even by name. And every reason for "nothing to resume" produced the same unhelpful message. All three are fixed, and the response now reports how many tables had already finished before the stop — that count was always zero, because the branch computing it couldn't be reached.

What operators will notice: a stop or cancel that can't write its table states now returns an error naming the database and the reason. It used to return success along with a count of writes that never happened. And a start refused because the change stopped longer ago than the search window now names that change and the exact command that resumes it anyway, instead of claiming there is nothing to resume.

Opened by Claude (Opus 5).

[Copilot]

Pull request overview

This PR hardens SchemaBot’s local control and dispatch logic to prevent tasks from becoming permanently “stranded” under terminal applies, and to ensure stop/cancel operations only report success when task state writes actually persist.

Changes:

• Expand orphan handling so tasks waiting under terminal applies are durably settled into an appropriate resting/terminal state, unblocking future dispatches.
• Make stop/cancel fail closed when lease-guarded task updates are refused, instead of counting refused writes as success and settling the apply anyway.
• Add targeted unit + integration coverage for stranded-task settlement and refused-write control-operation behavior.

Reviewed changes

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

Show a summary per file

File	Description
pkg/tern/local_orphaned_task_integration_test.go	Adds an integration scenario asserting a stranded retryable task under a stopped apply is settled and no engine work is touched.
pkg/tern/local_control.go	Updates stop/cancel to treat refused task writes as errors and only count/publish landed task updates.
pkg/tern/local_control_test.go	Adds tests ensuring stop/cancel fail when task writes are refused, and that counts reflect only persisted updates.
pkg/tern/local_apply.go	Generalizes orphan settlement beyond pending and introduces shared log message + settlement policy for terminal-apply tasks.
pkg/tern/local_apply_conflict_test.go	Adds conflict-check tests covering stranded retryable tasks across stopped/ended/claimable applies and write-failure behavior.

---

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

[JashLal]

Adversarial review — I went looking for ways this change is wrong rather than ways it is right. No blockers: the core fix (counting only landed writes, failing stop/cancel on refused task writes, settling stranded tasks under terminal applies) held up under the interleavings I traced. Findings, most substantive first.

1. IsTerminalTaskState doesn't normalize, and this PR's own premise makes that reachable

pkg/state/task.go:63; new call sites pkg/tern/local_control_resume.go:150, pkg/tern/local_apply.go:127

The PR asserts — with a test — that task states arrive in proto form (STATE_STOPPED) and fixes the raw comparisons for stopped. But IsTerminalTaskState is still a raw switch (unlike IsTerminalApplyState, which normalizes first), so a task stored as STATE_COMPLETED falls past the new terminal check at local_control_resume.go:150 into the unresumable bucket at line 160. Start then refuses with "tasks are in a state start cannot act on (STATE_COMPLETED)" instead of counting it skipped / "already reached a terminal state" — the same bug class this PR fixes, one branch over. The pre-existing loop head at local_apply.go:93 has the same latent shape (a STATE_CANCELLED row falls through to the engine-backed checks instead of being skipped as terminal). Suggest normalizing inside IsTerminalTaskState to mirror IsTerminalApplyState, or using state.IsState(task.State, state.TerminalTaskStates...) at the new call sites.

2. The stale > 0 branch of noResumableWorkError is unreachable

pkg/tern/local_control_resume.go:191

A named start sets ageBounded = false, so staleCount stays 0. An unnamed start only resolves an apply through an in-window stopped task in the discovery pass — and that same task increments startedCount in the second loop (same task slice, same window), so startedCount == 0 && stale > 0 can't occur. The carefully worded "re-issue it naming X" message can never be produced. TestLocalClient_StartOutsideTheDiscoveryWindowNamesTheWayForward quietly confirms this: despite its name, it asserts the generic discovery failure ("no stopped schema change to resume"), not this message. The place an operator actually hits the window is discovery returning nil, which still emits the bare message with no pointer to the out-of-window apply — the hint only lands in a server log they can't see. Consider having discovery remember the out-of-window candidate it passed over and surface the hint there, then delete the dead branch.

3. settleOrphanedTask settles without the guards its sibling treats as load-bearing

pkg/tern/local_apply.go:395 vs restingTaskReleasesDatabase at local_apply.go:467

restingTaskReleasesDatabase refuses to act on a terminal apply that still has a fresh lease ("a driver mid-settlement of this same apply") or a pending start/cancel. settleOrphanedTask checks only the apply state and then writes — and the conflict-check path holds no lease, so the write is unguarded. A driver mid-settlement (between its apply write and its task writes) can be writing the same row concurrently. Traced interleavings: stop-vs-settle converges on stopped, and a settled-to-stopped task still passes the resting guards before the database is released, which bounds the damage — but failApplyWithTasks-vs-settle means the last writer decides whether the row reads failed or cancelled. The old pending-only code had the same gap; extending settlement to failed_retryable widens it, since that's exactly a state a mid-settlement driver touches. Cheap mitigation: check apply.HasFreshLease(time.Now()) before settling, same as line 467. Was skipping it deliberate?

4. The settlement write drifted from persistTaskStateTransition

pkg/tern/local_apply.go:395 vs local_apply.go:767

persistTaskStateTransition clears ETASeconds/Throttled/ThrottleReason for at-rest states so "the stored row never carries a frozen estimate or renders as paused with no copy in flight." The hand-rolled write in settleOrphanedTask doesn't, so a task settled to stopped keeps whatever those fields held. Low severity for rows written by current code (the transition into failed_retryable already cleared them) — but the rows this sweep exists to repair are precisely the ones written by older or broken paths, so I wouldn't rely on that. The hand-roll is understandable (the rollback must also restore ErrorMessage/CompletedAt, which the shared helper's entry snapshot would keep), but the ETA/throttle clearing should match.

5. "settle 1 of 2 tasks to stopped" reads as a success count

pkg/tern/local_control.go:1641

len(refused) is the failed count, but "settle N of M tasks" scans as "N succeeded". Suggest "failed to settle 1 of 2 tasks to stopped: …". The doc comment at line 1589 also still describes three return values.

6. Question: the retry story after a partial stop failure and a restart

Post-PR, a refused task write leaves: engine stopped, apply handle cancelled, some tasks stopped, apply still running-family. A same-process retry works (Spirit's tracked state returns "Already stopped"). But if the process restarts first, Stop returns the permanent "no active schema change to stop" (pkg/engine/spirit/control.go:48) and the retried stop dies at engine stop failed before reaching the task writes; recovery then re-claims the running apply and resumes the copy the operator asked to stop, until a later stop lands. Strictly better than the old silent success + permanent wedge — just confirming recovery-resumes-then-stop-again is the intended story for that case.

---

None of these undermine the fix. Counting only landed writes, attempting every task so a retry shrinks, preserving ErrApplyLeaseLost through errors.Join, letting a named start bypass the discovery window, and settling a stranded task to stopped rather than cancelled so the copy stays resumable are all the right calls — and TestLocalClient_StartRecoversAfterAStrandedTaskIsSettled, reproducing the full wedge and its recovery, is the best kind of regression test. Approving.

Adversarial review by claude-fable-5-xhigh (Amp) 🤖

[aparajon]

🤖 Addressing @JashLal's adversarial review. All six addressed — fixes in four new commits (no squash, per post-approval convention), each verified by mutation (fix reverted → new test fails → fix restored → green), plus a full make test-unit pass.

1. IsTerminalTaskState now normalizes (074ed71a) — the switch runs on NormalizeState(s), mirroring IsTerminalApplyState, which fixes both the new call site at local_control_resume.go and the latent loop head at local_apply.go:93 in one place. TestIsTerminalTaskState_NormalizesProtoPrefix pins every terminal state in both canonical and STATE_* form (and that STATE_STOPPED / STATE_FAILED_RETRYABLE stay non-terminal), and TestLocalClient_StartSkipsProtoFormattedTerminalTasks pins the operator-visible half: a STATE_COMPLETED task now refuses with "already reached a terminal state", not "a state start cannot act on".

2. The hint moved to where operators hit the window (04a9ef15) — exactly as you suggested: discovery remembers the most recent stopped task it passed over for age alone, and when nothing lands in the window the refusal names that task's apply and the start invocation that resumes it anyway. The unreachable stale > 0 branch (and staleCount) are deleted, and the generic refusal now says how many tasks were seen and that none stopped inside the window. TestLocalClient_StartOutsideTheDiscoveryWindowNamesTheWayForward now earns its name — it asserts the apply identifier and the "re-issue the start naming" hint instead of the generic message.

3. Skipping the lease guard was not deliberate — added (af98324e) — settleOrphanedTask now defers when apply.HasFreshLease(time.Now()), same as restingTaskReleasesDatabase: a fresh lease means a driver may be mid-settlement with task writes in flight, so the task keeps blocking and the sweep settles it on a later check once the lease has aged out. TestConflictCheckDefersStrandedSettlementToAFreshLease covers both halves (fresh lease → untouched; aged out → settled).

4. The settlement write matches persistTaskStateTransition's at-rest clearing (same commit af98324e) — ETASeconds/Throttled/ThrottleReason are cleared before the conditional write, and the refused-write rollback still restores the full prior row. TestConflictCheckClearsRestingFieldsOnStrandedSettlement pins it, including that ErrorMessage survives.

5. Message and doc comment fixed (095ef517) — the error now reads failed to settle 1 of 2 tasks to stopped: …, and the doc comment describes all four return values. The test pin asserts the "failed to settle" phrasing.

6. Confirmed — recovery-resumes-then-stop-again is the intended story. A partial stop failure leaves the apply running-family on purpose: the copy's true condition is "still recoverable, stop not fully landed", and recovery re-claiming it keeps the checkpoint warm rather than stranding it. The retried stop (post-restart, once a driver has re-claimed) then lands through the normal path. It costs the operator a second stop in a rare double-fault window, and that's strictly better than the old silent success that wedged the database permanently.

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