fix(tern): a refused stop or cancel no longer settles the apply stopped

[aparajon]

Why this matters

An operator can cancel a schema change that has already cut over and is holding its revert window. Nothing gates the command, and the drive refuses it there, because at that point only revert (undo) or skip-revert (finalize) can act. That refusal is correct and it resolves the durable request with the reason.

Then it told the drive the stop had taken effect. The drive stood down, marked the cut-over task stopped in memory, and settled the apply stopped in storage. The change was applied. The engine was still working underneath. And the revert window was left with no driver watching it expire.

Before                                    After

┌────────────────────────────┐            ┌────────────────────────────┐
│ cancel arrives in the      │            │ cancel arrives in the      │
│ revert window              │            │ revert window              │
└─────────────┬──────────────┘            └─────────────┬──────────────┘
              │                                         │
              ▼                                         ▼
┌────────────────────────────┐            ┌────────────────────────────┐
│ request failed with the    │            │ request failed with the    │
│ revert-phase reason        │            │ revert-phase reason        │
└─────────────┬──────────────┘            └─────────────┬──────────────┘
              │ reported: it took effect                │ reported: no effect
              ▼                                         ▼
┌────────────────────────────┐            ┌────────────────────────────┐
│ drive stands down          │            │ drive keeps polling        │
└─────────────┬──────────────┘            └─────────────┬──────────────┘
              │                                         │
              ▼                                         ▼
┌────────────────────────────┐            ┌────────────────────────────┐
│ apply settles stopped over │            │ the revert phase settles   │
│ a live revert window       │            │ its own outcome            │
└────────────────────────────┘            └────────────────────────────┘

Reachable with no operator error and no unusual timing. A task enters its revert window, the progress tick persists that state, and the next tick reads a pending cancel: the gate fires, and the tick that had been watching the change walks away from it reporting a pause that never happened. The engine that reports a revert window always runs its tables as a group, so the grouped progress tick is where this lands.

What it does

Both revert-phase gates now report that nothing took effect. The drive is left exactly as the gate found it, which is the whole point of a refusal: the poll keeps polling, and the revert phase drives its own outcome. The durable request is still resolved, so no later claim re-sends it.

The value the processors return conflated independent facts about one request. "I resolved this request" and "the drive should stand down" are not the same claim, and a gate that resolves a request could look right while returning the opposite of what the drive needed. Nor is standing down the same as the command taking effect: a completed stop with a start already queued keeps the drive going so the same claim resumes from the start. So the return is now a named standDown on both clients, documented by the one question it answers rather than by the branches that reach it.

Two existing tests asserted the old value in order to express "the request is resolved" — one of them says so in its own assertion message. That fact is asserted against the stored request now, which is where it lives.

Invariants

Upholds CO-5 ("the revert phase owns the outcome"). Its rule already decides this case, and its stated reason is the defect exactly: storage must never settle on a state that contradicts what the engine is still doing to the database underneath. The gate that exists to enforce CO-5 was contradicting it one line later.

The entry itself needed two corrections, neither of them a change to the rule. It named reverting and skipping_revert but not the revert window, which is the phase every one of these refusals is actually reached in, so a reviewer following the citation arrived at an entry about different states. And its *Enforced:* line named only the control path, while the half this change fixes lives in the drive loops that act on the gate's answer.

Upholds ST-7 ("stop checkpoints conservatively"): the task snapshot is taken only after the engine's own stop returns, and here no engine stop was ever attempted. Its text and *Enforced:* line are unchanged.

Opened by Claude (Opus 5).

[Copilot]

🟡 Changes recommended

The new “tookEffect” contract is documented as always resolving requests, but the implementation can intentionally leave requests pending, and there are still error paths in the changed refusal branches that return tookEffect=true, which is inconsistent with the new semantics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes control-request handling in the tern drive loop so that a refused stop/cancel during the revert window no longer causes the driver to stand down and settle the apply as stopped, preserving CO-5 (“the revert phase owns the outcome”) and ST-7 (“stop checkpoints conservatively”).

Before (bug)                                 After (fixed)

refused stop/cancel in revert window          refused stop/cancel in revert window
        │                                             │
        ▼                                             ▼
drive treated as “took effect”                 drive treats as “no effect”
        │                                             │
        ▼                                             ▼
driver stands down + apply settles stopped      driver keeps polling; revert window settles

Changes:

• Change stop/cancel processing to return “took effect” = false for revert-phase refusals, so the driver keeps polling and the revert phase settles its own outcome.
• Update sequential/grouped/resume drive loops to use the renamed tookEffect signal.
• Add/adjust tests covering revert-window refusals and ensuring durable requests are resolved without stopping the drive.

File summaries

File	Description
pkg/tern/local_control.go	Adjusts local stop/cancel control-request consumption so refusals in revert phases don’t signal a stop/cancel took effect.
pkg/tern/local_control_test.go	Refactors client construction to inject control-request stores; adds targeted revert-window refusal tests.
pkg/tern/local_control_resume.go	Updates resume loop to use the new tookEffect semantics when deciding to stand down.
pkg/tern/local_client_test.go	Updates stop-revert-window rejection test expectations to match “no effect” semantics.
pkg/tern/local_apply_sequential.go	Updates sequential apply loop and polling loop to use tookEffect when deciding to stop driving.
pkg/tern/local_apply_sequential_progress_test.go	Adds a progress/poll test asserting a refused cancel in revert window keeps polling until engine completion.
pkg/tern/local_apply_grouped.go	Updates grouped apply flow to use tookEffect when deciding whether to return early.
pkg/tern/grpc_client.go	Mirrors the stop/cancel “tookEffect” contract changes in the gRPC client call sites and documentation.

Review details

Suppressed comments (1)

pkg/tern/local_control.go:1267

• Same issue as the stop refusal path: if failing the pending cancel request write returns an error, the cancel did not take effect and the request is still unresolved, so the error path should return tookEffect=false rather than true.

		if err := failPendingControlRequests(ctx, c.storage, apply, storage.ControlOperationCancel, message); err != nil {
			return true, err
		}

• Files reviewed: 8/8 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 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/1330, 1f086ef.

Verdict: 7 findings — 0 blocking, 3 non-blocking, 4 suggestions. The fix itself is right and now well pinned: reverting all four flipped returns kills six tests, and before this PR nothing in ./pkg/tern/ noticed the flip at all. Everything below is about the contract comments the PR adds — the gRPC copy describes branches that return the opposite of what it claims — plus test-symmetry gaps.

Non-blocking

1. The new gRPC contract comment is wrong about the code it sits above, in three places. grpc_client.go:848 says a refusal "resolves the request … and reports false, leaving the drive exactly as it found it", but an operation-only refusal deliberately leaves the request pending (:816-825) — as failRefusedControlRequest's own comment states at :804 — and both refusal branches mutate the send gate. Its headline example of "reports true … an accepted stop whose apply row has not settled" is the one branch whose log literally reads accepted and remains pending and which returns false, nil at :1009; that shape exists on the local client (local_control.go:1221-1231) and looks carried across. "Two shapes" also undercounts: 883, 902, 981 and the settle path at 1168 are four true-with-pending returns, all operation-only.

2. The gRPC refusal still reports true when the resolving write fails — the exact shape the PR flipped locally. grpc_client.go:839 returns true, fmt.Errorf(...), where the local twins became false, err (local_control.go:1184, :1272) under the PR's own rule that "the refusal already decided that nothing was stopped". Not a live bug — I checked all ten gRPC call sites and every one returns on a non-nil error before reading the boolean — but it is the last copy of the pattern this PR set out to delete, and it now contradicts the comment 9 lines below it.

3. false does not actually mean "the stop took effect is false". stopHandledUnlessStartPending returns false, nil at :1057 for a stop that did take effect on a terminal apply, when a start is queued — reached from :1153 and :1164, and mirrored at grpc_client.go:895. Both new comments present their enumeration as exhaustive and omit it, and the return really means "the drive must not stand down", which is the useful reading but not the one written. The rename also stopped at the three functions: "handled" survives at :1043, :1044, :1072, :1107 and — mixing both vocabularies in one sentence — at :1214 and :1298.

General suggestions

4. One of the four flipped returns is unpinned. Flipping only the cancel side's failPendingControlRequests-error return at local_control.go:1272 back to true leaves the whole package green (ok … 63.309s), while its stop twin at :1184 is killed by the new …ReportsNoEffectWhenResolvingTheRequestFails. It is an equivalent mutant — every caller is error-dominant — so this is symmetry, not coverage of behavior; revertWindowRefusalFixture(storage.ControlOperationCancel) plus failPendingErrorStore make the twin about 12 lines.

5. The drive that actually consumes the new false has no drive-level test. GroupsEngineExecution returns true unconditionally for Vitess (storage/types.go:1191) and only the PlanetScale engine emits StateRevertWindow, so the reachable site is handleAtomicProgressTick (local_apply_grouped.go:636, :785) — and every existing tick test stages an empty &testControlRequestStore{}, so none reaches a refusal. The new TestPollTaskToCompletion_RefusedCancelInTheRevertWindowKeepsPolling pairs the sequential poll with a state only a custom EngineFactories type could produce there; a tick test with a pending cancel asserting done == false would put the demonstration on the real path.

6. executeApplySequential lacks the fail-closed guard its resume twin has. local_control_resume.go:2113-2118 refuses a sequential resume holding any revert-phase task; the fresh-dispatch loop at local_apply_sequential.go:48 now continues to the next table instead. Latent only — that drive cannot hold a revert-window task for any built-in type — and the old true was strictly worse there (it settled the apply stopped), so this is a defensive symmetry suggestion, not a regression.

7. Two smaller ones. CO-5's text names only reverting/skipping_revert, while the gate the PR relies on (applyRevertPhaseBlock:1407-1418) also fires on revert_window and on task state — which is exactly what every new test stages, so a reviewer checking the citation reads an entry about a different state; "upholds" is still the right disposition per an entry is a principle, not a case log, but CO-5's *Enforced:* line names only local_control.go and the half this PR adds lives in the three drive files. And :1184/:1272 return bare err where their own neighbours at :1205/:1291 wrap with the apply identifier.

The one thing that could have broken, verified

resumeApplyWithTasks :1889 used to return nil on a refusal and now falls through into plan loading and launchAtomicResume — for an apply in its revert phase. I tried to build the state that re-deploys or clobbers task state and could not. The load-bearing guard is shouldInspectCutoverSignalForResume (:1037-1043), which requires waiting_for_cutover/recovering and so keeps markApplyRecovering's task rewrite out of reach; replanAndFilterTasks skips revert-phase tasks ahead of both persistTaskStateTransition calls (:665-678), persistReattachedResumeStates preserves the phase (:1309, :1323), and the grouped path fails closed with no write when resume state is missing (:1397-1400). Decisively: the identical fall-through already happens on every routine claim with no pending request (local_control.go:1140, :1246 return false, nil), so the PR does not open the path — it stops a refusal from suppressing it.

Verified correct

• Reverting all four flipped returns kills exactly six tests and nothing else in the package — i.e. before this PR the flip was completely unpinned, and after it, it is pinned six ways.
• Per-half mutants die cleanly: the cancel half kills only the three cancel tests, the stop half only the three stop tests, and the stop FailPending test is the sole killer of :1184.
• The refusal gate really is reached by the new fixtures — the logger.Warn inside the branch (:1175, :1265) fired with revert_phase=revert_window for every new test, so none passes on an earlier short-circuit.
• TestPollTaskToCompletion_RefusedCancelInTheRevertWindowKeepsPolling kills the reverted fix three independent ways (action, task.State, and the recorded state sequence), not on one equality.
• failPendingErrorStore's embedding is sound and the test is not vacuous: GetPending/GetByOperation promote to the real fake, so its assertions read unmutated state.
• A resolved refusal is consumed exactly once — GetPending filters status = pending (sqlstore/control_requests.go:117-124) and FailPending writes failed (:174-208) — so the per-tick site cannot become a refuse/log loop.
• All eight non-test call sites read err before the bool, so true, err → false, err is behavior-neutral; no caller relied on the old "request handled" meaning.
• The gRPC portion is rename-only: that client has no revert-phase gate at all, and its refusals already returned false via failRefusedControlRequest.
• Full ./pkg/tern/ passes at this head (63.2s), gofmt -l is clean on all eight changed files, and CI is green including Integration, LocalScale, Terminology and Templates drift.
• Conventions met: testify require/assert split, t.Context() throughout, a scenario comment above each new test, and the new comments are present-tense contract docs rather than change narration.

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

[aparajon]

🤖 Replying to the review at 1f086ef9 — thanks. Six of the seven are fixed in 556ad13, and one of them corrected my own account of where this is reachable.

1, 3 — the contract comments, and the name. Both accepted, and they turned out to be one problem. stopHandledUnlessStartPending returning false for a stop that did take effect means tookEffect was wrong about its own code, not just under-documented. The return is now standDown on both clients, and each doc comment states the single question it answers instead of enumerating the shapes that reach it — the enumerations were the part that went stale, and mine described the local client's branches while sitting above the gRPC one. handled is gone from the surrounding comments and from the resend tests too.

2 — the gRPC refusal's write-error return. Fixed, and pinned: stopRefusal on capturingTernServer plus a store whose FailPending fails reaches it, so it is no longer unpinned by construction.

4 — the unpinned cancel twin. Added, and it dies on the mutant.

5 — the drive-level test was on the wrong path. This is the one I had wrong in the PR body, not just in the tests. Only PlanetScale reports a revert window and PlanetScale always groups, so the sequential poll I described as the reachable site cannot see one from any built-in engine. There is now a handleAtomicProgressTick test with a pending cancel asserting done == false, and the body says grouped tick. The tick also syncs the apply to revert_window on that pass, which is a sharper assertion than the one I first wrote: the apply tracks the phase the engine reported, not the command that was refused.

7 — CO-5 and the bare errors. Both fixed. CO-5 now names the phase rather than two of its three states, and its *Enforced:* line names the drive loops alongside the control path. Both refusal writes wrap with the apply identifier like their neighbours.

6 — the sequential fail-closed guard: declining. The distinction I'd draw is that a resume adopts whatever task state storage holds, so its guard can be reached by state it did not write; a fresh sequential dispatch creates its tasks, and per finding 5 no built-in engine can put a revert window under it. A refusal there would be a branch no test can reach, which is what I'd rather not add. If you think the resume guard's reachability argument extends further than that, I'll take it.

Also worth recording: your census found that before this PR the flip was completely unpinned in ./pkg/tern/. That is the more useful fact about the original defect than its reachability, and I hadn't checked it.

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