fix(tern): fail resume re-plan closed when task settlement write is refused

[Kiran01bm]

Task-settlement paths now fail closed when the durable write settling a task as completed is refused, instead of counting the task settled and terminalizing the parent apply — or resolving a durable control request — over a task row that never changed.

Why

When a drive resumes an apply and the re-plan finds a task's table gone from the diff (the engine finished the change before the previous drive lost the apply), it settles the task as completed and the caller derives the parent apply's terminal state from that partition. The settlement write is lease-guarded, and two drivers racing to claim a stale operation can leave the winning drive holding an operation-lease token a peer has since rotated. The refused write was logged and swallowed, so the re-plan still counted the task completed and the drive marked the apply completed — stranding a durable contradiction: apply = completed while the task row stays failed_retryable.

What

• replanAndFilterTasks now persists the completed settlement through a returning path and aborts the resume with an error when the write is refused, releasing the drive so a later claim redoes the settlement under a current lease.
• The two remaining settlement sites with the same swallow-then-terminalize shape are converted the same way: the sequential resume's raced-cutover settle aborts the resume without finalizing, and the completed-on-engine stop/cancel settle fails the settle so the durable control request stays pending for a later claim.
• transitionTaskState is split into a swallowing wrapper (best-effort progress paths) over a new persistTaskStateTransition that returns the storage error and only records the apply-log event after the write lands. This changes behavior at every wrapper call site: a failed best-effort write no longer emits an apply-log event, so the durable log never claims a transition the task row does not carry.
• Unit tests cover the success settlements, each fail-closed path, and pin the log-write ordering: a refused write records no state-transition event.

Before / after

Before: refused settlement write is swallowed
┌───────────────────────────────┐
│ settle task completed         │
│ (re-plan / raced cutover /    │
│  completed-on-engine stop or  │
│  cancel)                      │
└──────────────┬────────────────┘
               │ Tasks().Update refused (lease lost to peer)
               ▼
   log error, keep going ──▶ count task completed
               │
               ▼
   apply marked COMPLETED / control request resolved
               │
               ▼
   durable state: apply=completed, task=failed_retryable  ✗

After: refused settlement write aborts the settle
┌───────────────────────────────┐
│ settle task completed         │
│ (re-plan / raced cutover /    │
│  completed-on-engine stop or  │
│  cancel)                      │
└──────────────┬────────────────┘
               │ Tasks().Update refused (lease lost to peer)
               ▼
   return error ──▶ drive aborts (no apply write,
               │    control request stays pending)
               ▼
   later claim, fresh lease ──▶ settlement lands ──▶ task completed,
                                                     then apply completed  ✓

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[Copilot]

Pull request overview

This PR tightens SchemaBot’s tern resume behavior to fail closed when a task settlement update (marking “no remaining work” as completed) is refused by storage (e.g., due to a lease-guarded write), preventing the parent apply from being terminalized based on a task state that did not durably persist.

Changes:

• Make replanAndFilterTasks persist “settled completed” task transitions via a returning path and abort the resume re-plan on refused writes.
• Split task state transitions into a best-effort wrapper (transitionTaskState) and a strict persistence helper (persistTaskStateTransition) that only records the durable apply-log event after the task row update lands.
• Add unit tests covering both the successful settlement and the fail-closed error path.

Reviewed changes

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

File	Description
pkg/tern/local_control_resume.go	Fail closed when persisting a “table left diff → settle completed” task transition is refused during resume re-plan.
pkg/tern/local_control_resume_test.go	Add unit tests for successful settlement persistence and refused-write fail-closed behavior.
pkg/tern/local_apply.go	Introduce persistTaskStateTransition and make transitionTaskState a log-and-swallow wrapper that no longer writes a durable log event unless the task update succeeds.

---

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

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head de0d2ef8, in a worktree, with ./pkg/tern/... built and run locally and a mutation battery against the new tests.

Verdict: clean — nothing blocks. The diagnosis is right and the shape of the fix is right: the settlement write is what the caller's partition is derived from, so swallowing it lets adoptSequentialOutcome's default branch mark the apply completed over a task row that never moved. Splitting a returning helper out from the swallowing wrapper, rather than making all twenty callers fail closed, is the correct scope. Both halves of the new behavior are pinned. The gap is that this is one of three sites with the identical shape, and the second one is a single line away from the same fix.

#	Finding	Severity
1	Two more settlements swallow the same write and reach the same terminal apply write	correctness (pre-existing)
2	The log-ordering invariant the doc comment states is unpinned	coverage

---

1. The same swallow survives at two other settlements, both of which go on to terminalize the apply

Copilot's open thread names one of them. There's a third it didn't see, and it's the easier of the two to fix.

settleControlForCompletedEngineChange (pkg/tern/local_control.go, ~1698) is nearly line-for-line the site this PR converted — same three lines, same completedCount++:

task.ProgressPercent = 100
task.CompletedAt = &now
c.transitionTaskState(ctx, task, task.ApplyID, state.Task.Completed, ...)
completedCount++

and then, a few lines down, loads the apply and writes it completed. It is already func(...) (int64, error), so the conversion is one if err := c.persistTaskStateTransition(...); err != nil { return 0, fmt.Errorf(...) } with no signature change and no new plumbing. Its own doc comment makes the stakes explicit — "the accepted settle lets the durable control request resolve" — so a refused write there doesn't just strand the contradiction, it resolves the operator's control request on the strength of a settlement that didn't land, and the claim loop stops retrying.

resumeApplySequential's !needsChange branch (pkg/tern/local_control_resume.go, ~368) is the one Copilot flagged, and the trace holds: finalizeSequentialApply reloads the apply, sees it non-terminal, calls adoptSequentialOutcome — which consults only failedTask and stoppedByUser and otherwise falls to default: Completed — and writes it. Same contradiction, reached the same way. That branch also has no test on it at all: deleting the settlement outright leaves ./pkg/tern/... green.

Neither is a regression and neither blocks — the PR is a strict improvement and the diff is well scoped. But the body reads as though the class is now closed ("instead of counting the task settled and terminalizing the parent apply over a task row that never changed"), and two of the three sites still do exactly that. If they're being left for a follow-up, worth saying so; settleControlForCompletedEngineChange in particular is cheap enough that leaving it feels like more work than taking it.

2. "The durable log never claims a transition the task row does not carry" isn't held by anything

That's the sentence the new doc comment leads with, and it's the reason the helper orders the write before logApplyEvent. Reversing the order:

c.logApplyEvent(...)                     // now runs first, unconditionally
if err := c.storage.Tasks().Update(...); err != nil { return ... }
  → go test ./pkg/tern/...   green

Nothing notices. The rest is well pinned — reverting the resume site to the wrapper, or swallowing inside the new helper, each kills TestReplanAndFilterTasks_FailsClosedWhenCompletedWriteRefused immediately, and dropping completedCount++ kills TestReplanAndFilterTasks_SettledTaskPersistsCompleted. It's specifically the ordering that floats, and this is the property that matters to the operator reading the apply log during triage: a log event asserting stopped → completed for a task row still sitting in failed_retryable is the thing that sends someone down the wrong path. The fail-closed test already uses a storage double that refuses the update — asserting no state-transition event was recorded on that same call is a couple of lines on a test that already exists.

Also

(nit) The body calls the wrapper "unchanged behavior" in the parenthetical and then describes the log-event change in the same sentence. It is a real behavior change, and it lands on all twenty wrapper call sites, not just the converted one — a best-effort progress transition whose write fails no longer emits its apply-log event either. That's the better behavior and it's the right call; it just isn't "unchanged", and it's the kind of line a reader skims past.

---

Action items

1. (Finding 1) Convert settleControlForCompletedEngineChange — it already returns an error — and take Copilot's thread on resumeApplySequential's !needsChange branch, or say in the body that they're a follow-up.
2. (Finding 2) Assert in the fail-closed test that no state-transition event was recorded when the write was refused.
3. Reply to and resolve the open Copilot thread either way.

---

Verified — tried to break, couldn't

The mutation results:

Mutation	Result
the new helper swallows the storage error again	🔴 TestReplanAndFilterTasks_FailsClosedWhenCompletedWriteRefused
the resume settlement goes back through the swallowing wrapper	🔴 same test
the settled task is no longer counted	🔴 TestReplanAndFilterTasks_SettledTaskPersistsCompleted
the apply-log event is recorded before the write	🟢 survives — Finding 2
resumeApplySequential never settles the raced task at all	🟢 survives — no coverage on that branch
settleControlForCompletedEngineChange never settles its tasks	🔴 TestLocalClient_ProcessPendingCancelSettlesCompletedEngineChange, …Stop… (happy path covered; the refusal path isn't)

The contradiction the PR describes is genuinely reachable. adoptSequentialOutcome derives the apply's terminal state from failedTask and stoppedByUser only — it never reads a task row — and its default is Completed. finalizeSequentialApply reloads the apply first and declines to overwrite a state that's already terminal, but a non-terminal apply gets the derived state written. So a swallowed settlement really does produce apply = completed over a non-terminal task, with nothing downstream to catch it.

The lease window is the right one to worry about. Tasks().Update prefers the operation lease over the apply lease and Applies().Update refuses an operation-lease-only context with ErrApplyLeaseLost, so the dangerous shape is narrow: a single-operation drive holding both leases where only the operation token was rotated. That's exactly the case the body names, and it's the case where the apply write would still succeed after the task write was refused.

Aborting the resume is safe for the caller. Both replanAndFilterTasks callers return immediately on error and neither reuses the partially-mutated tasks slice, so the in-memory State/ProgressPercent/CompletedAt that persistTaskStateTransition sets before the write — and doesn't roll back on failure — can't leak into a later decision. Worth knowing if a third caller ever appears, but contained today.

The wrapper's contract is documented where it needs to be. The comment on transitionTaskState now says why the swallow exists (best-effort progress paths, next poll retries) and points at the returning helper for callers whose control flow depends on the write. That's the distinction a future caller has to get right, and it's stated at the call site rather than in the PR.

Ran locally at head: go build ./... and ./pkg/tern/... green (62s). CI 34/34. No test deletions or weakened assertions in the diff. Leak check on the body and diff clean, terminology clean.

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

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) code review assessment agent (Amp / Claude Opus 4.5)

Summary: All findings accepted and fixed in the follow-up commit — both remaining swallow-then-terminalize settlement sites now fail closed, the log-ordering invariant is pinned by tests, and the PR body wording is corrected.

#	Finding	Status	Explanation
1	Two more settlements swallow the refused write and reach the same terminal apply write (settleControlForCompletedEngineChange; resumeApplySequential's !needsChange branch)	fixed	Both converted to persistTaskStateTransition: the stop/cancel settle returns the error so the durable control request stays pending for a later claim; the sequential resume logs and aborts without finalizing, leaving the apply claimable. New tests TestLocalClient_ProcessPendingCancelFailsClosedWhenSettleWriteRefused and TestResumeApplySequential_AbortsWhenRacedCutoverSettlementRefused (the latter branch previously had no coverage).
2	The log-ordering invariant ("durable log never claims a transition the task row does not carry") is unpinned — reversing the write/logApplyEvent order survives the suite	fixed	All three fail-closed tests now assert no state-transition apply-log event is recorded on a refused write; TestReplanAndFilterTasks_SettledTaskPersistsCompleted asserts the event IS recorded once the write lands.
nit	PR body calls the wrapper split "unchanged behavior" — it is a real behavior change landing on every wrapper call site	fixed	PR body updated: it now states that a failed best-effort write no longer emits its apply-log event at any wrapper call site, and that the two additional settlement sites are converted.
—	Reply to and resolve the open Copilot thread	fixed	Replied and resolved; it flagged the same resumeApplySequential branch as finding 1 and is addressed by the same commit.
—	"Verified — tried to break, couldn't" mutation battery (confirmations only)	no action	Confirmations, no findings.