fix(tern): sequential operation-scoped drives leave the parent apply to the projection

[aparajon]

Why this matters

When one schema change targets multiple deployments, SchemaBot creates a single apply with one operation per deployment and drives each operation separately. A driver working on one operation owns only that operation — the shared parent apply row belongs to the operator, which watches all the operations and derives the parent's state from their outcomes. Storage enforces this ownership: a driver holding only an operation lease gets refused if it tries to write the parent row directly.

The grouped resume path respects that rule. The sequential resume path did not: the first thing it did on every drive was write "running" onto the parent row. Storage refused the write, the drive gave up before starting any task work, and the operator re-claimed the operation on the next tick and failed the exact same way. The schema change made no progress, forever:

operator claim ──▶ op-scoped drive ──▶ parent write ──▶ refused (op lease only)
      ▲                                                        │
      └──────────────── re-claim, once per tick ◀──────────────┘

What it does

The sequential path now behaves like the grouped path. A driver holding only an operation lease:

• drives its own tasks and records each task's outcome — completed, failed, or cancelled behind a failure;
• skips every parent-row write (mark running, mark completed, control-request resolution, apply-level metrics, the terminal PR summary), leaving all of those to the operator, which derives them from the recorded task outcomes;
• exits cleanly even when the engine fails the work — the failure is already durably recorded on the tasks, and exiting with an error would make the operator treat the drive as transiently broken and keep re-claiming work that is already settled.

Integration tests drive one operation of a two-operation apply through three endings — success, nothing left to do after a re-plan, and an engine failure — and verify the tasks settle while the parent row is never written by the drive.

How it moves us toward the northstar

The northstar puts one data-plane apply behind each deployment, so several operations sharing one parent apply is the normal shape of every multi-deployment schema change — not an edge case. This closes the last drive path that could not make progress under the operator-owned parent model.

Opened by Claude (Fable 5).

[Copilot]

Pull request overview

This PR fixes operation-scoped drives for multi-operation applies by making the sequential resume path projection-aware: under an operation-only lease, the drive now settles only its own tasks and avoids parent applies writes that storage correctly rejects, leaving parent-state updates and terminal UX to the operator’s rollout projection.

Changes:

• Skip parent apply state updates and parent control-request completion during sequential resume when running under an operation-only lease.
• Ensure terminal-truth reconciliation adopts engine outcomes onto tasks without writing parent terminal state under an operation-only lease.
• Add integration coverage that drives an operation-scoped sequential resume through success, “no remaining work” re-plan, and engine rejection while asserting the parent apply row remains untouched.

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_control_resume.go	Makes sequential resume and recovery paths suppress parent apply writes under operation-only leases.
pkg/tern/local_control_reconcile.go	Suppresses parent terminal writes and pending-command completion during terminal-truth adoption for operation-scoped drives.
pkg/tern/local_control_multiop_resume_integration_test.go	Adds integration tests validating operation-scoped sequential resume settles tasks without mutating the parent apply row.
pkg/tern/local_apply_sequential.go	Updates sequential finalization to settle/cancel tasks only (no parent apply writes) when operation-scoped.
pkg/tern/local_apply_failure.go	Updates failure/retryable paths to settle tasks only (no parent apply writes/metrics/log stream) when operation-scoped.

---

💡 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/1074, 62c516a.

Verdict: 7 findings — 2 blocking (unconditional parent-apply writes still reachable under an operation-lease-only ctx), 3 non-blocking, 2 general suggestions.

Blocking

• pkg/tern/local_control_resume.go:273 — c.storage.Applies().Update(ctx, apply) inside processPendingStartControlRequest, called unconditionally from resumeApplyWithTasks (line 1546) before suppressParentApplyWrites is even computed. If a multi-op apply is WaitingForDeploy with a pending Start request when an operation-lease-only drive ticks (a state the operator's own claim query explicitly treats as reclaimable, per apply_operations.go:800-810), storage refuses the write with ErrApplyLeaseLost and the drive errors out — repeating indefinitely on every reclaim.

• pkg/tern/local_control.go:1672 — c.storage.Applies().Update(ctx, apply) in settleControlForCompletedEngineChange, reached from stopOwnedApply/cancelOwnedApply whenever eng.Stop()/eng.Cancel() returns engine.IsAlreadyCompleted, with no suppressParentApplyWrites guard. A Stop/Cancel racing the engine's own completion on a multi-op, operation-lease-only drive fails the same way instead of settling the apply as completed.

Non-blocking

• pkg/tern/control_requests.go:120 — if err := applyStore.Update(ctx, apply); err != nil { in markApplyCuttingOverForControlRequest, reachable from handleAtomicProgressTick under the same operation-lease-only ctx as a multi-op atomic drive, with no suppression check. A deferred-cutover trigger racing that drive fails closed instead of landing the cutover; also evidence the PR's own ~24-site grep inventory undercounts sites where the store accessor is captured via a local variable (applyStore := store.Applies()) rather than chained inline.

• pkg/tern/local_control_reconcile.go:109 — if suppressParentApplyWrites(ctx) { return true, nil }. This branch, its adoptEngineTerminalTruth sibling (line 232), and the retryable-engine-error branch of handleGroupedResumeFailure (via markApplyRetryableWithTasks) have no integration coverage under an operation lease — the 4 new tests in local_control_multiop_resume_integration_test.go never drive a retryable engine error or an engine-terminal-truth reconcile in that mode, so a future refactor could silently break the early-return here.

• pkg/tern/local_control_resume.go:393 — logger.Info("sequential resume finished", "state", apply.State) always reports state=Running for an operation-scoped drive: apply.State = state.Apply.Running is set unconditionally at line 1677, and finalizeSequentialApply's suppress branch (local_apply_sequential.go:651-662) returns early without ever reassigning it. Confirmed log-only staleness — no downstream code reads/branches on this stale value — but it's misleading during triage of a failed/stopped sequential drive.

General suggestions

• pkg/tern/local_control_resume.go:1618 — the new suppress branches around deferred-cutover recovery and handleGroupedResumeFailure call failApplyWithTasks (which already logs and returns early when suppressed) and then immediately log a second, differently-worded message before returning. Not incorrect, just adjacent double-logging that's slightly noisier than necessary for an operator grepping the apply log stream.

• pkg/tern/local_control.go:1994 — FYI, verified safe rather than a gap: recordAppliedVolume is the one remaining untouched Applies().Update site, but processPendingVolumeControlRequest fails multi-operation applies closed at line 1926 before reaching it, and single-operation drives always carry a dual (apply+operation) lease, so suppressParentApplyWrites is never true here.

The one thing that could have broken, verified

The riskiest mechanism is resumeApplyWithTasks's restructuring around the new if suppressParent { ... } else { ... } split (local_control_resume.go:1682-1697) — a bad refactor could nest the actual task-driving call inside the else, meaning an operation-scoped drive under suppression would never drive any task work at all (the exact bug this PR claims to fix, just relocated). Verified by reading lines 1674-1707: resumeCtx, cancelResume := context.WithCancel(ctx) and c.resumeApplySequential(resumeCtx, apply, activeTasks, plan, options) (lines 1699-1703) sit outside and after the if/else block, so resumeApplySequential runs unconditionally regardless of suppressParent's value.

Verified correct

• Every new suppress branch settles/writes the task row before checking suppressParentApplyWrites — confirmed across local_apply_failure.go:58-62,106-110, local_apply_sequential.go:651-662, local_control_reconcile.go:109-111,232-236, and local_control_resume.go:1618-1621,1642-1645,1682-1697 — so no task row is left stale while the parent write is skipped.
• finalizeSequentialApply's new suppress branch and the pre-existing non-suppress case failedTask != nil: pending-task-cancellation loop (local_apply_sequential.go:651-661 vs 678-697) are behaviorally equivalent.
• Pre-existing grouped-path suppression wiring (launchAtomicResume, persistReattachedResumeStates, markApplyRecovering) is untouched by this diff and was already correctly gated.
• Every new suppress early-return logs via logger.Info naming what the operator does instead (AGENTS.md "No silent branch cases"); no raw-string state comparisons or "fix for"/"previously"-style comments were introduced.
• The new integration test file uses testify assertions and t.Context() throughout, and each of the 4 tests carries a human-readable scenario/expected-outcome comment per AGENTS.md's "Explain scenario tests" convention.

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

[aparajon]

🤖 All seven findings addressed in fdccef5:

• Blocking 1 (processPendingStartControlRequest) — the drive still consumes the pending start (the deferred-deploy engine dispatch and task drive are its to own), but under an operation-only lease it skips the parent running write and leaves the request pending: sibling operations' deferred-deploy claim arm keys on the pending request plus the parent's waiting_for_deploy, so completing it from one deployment's drive would strand the others. Known follow-up: nothing completes multi-op start requests today (the projection completes stop/cancel only). Test: TestLocalClient_OperationDrivePendingStartTriggersDeployWithoutParentWrites.
• Blocking 2 (settleControlForCompletedEngineChange) — tasks still adopt the engine's completed truth; under an operation-only lease the parent reload/write, apply event, and terminal observer are skipped and the settle returns accepted, so the pending stop/cancel resolves through the projection's existing "request stays pending" path. Test: TestLocalClient_OperationStopAgainstCompletedEngineSettlesTasksNotParent.
• markApplyCuttingOverForControlRequest — now takes the caller's logger and skips the parent cutting_over write under an operation-only lease while keeping the in-memory transition, so the cutover still dispatches. Good catch on the grep inventory missing local-variable accessor sites.
• Coverage — added TestLocalClient_OperationGroupedResumeRetryableFailurePausesTasksNotParent (retryable branch under op lease) and TestLocalClient_OperationDriveAdoptsEngineTerminalTruthWithoutParentWrites (drives both local_control_reconcile.go suppress branches).
• Stale state=Running log — finalizeSequentialApply adopts the settled outcome into the in-memory apply under suppression (extracted adoptSequentialOutcome), so "sequential resume finished" reports the true state.
• Double-logging — dropped the caller-side Info at both sites; failApplyWithTasks's suppress log is the single line.
• recordAppliedVolume — agreed, no change; the multi-op gate fails closed before it.

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