Skip to content

fix(storage): terminal apply states are immutable - #644

Draft
aparajon wants to merge 1 commit into
mainfrom
armand/terminal-state-guard
Draft

fix(storage): terminal apply states are immutable#644
aparajon wants to merge 1 commit into
mainfrom
armand/terminal-state-guard

Conversation

@aparajon

@aparajon aparajon commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Why this matters

Applies().Update was last-writer-wins: any caller holding a stale in-memory snapshot could write an active state over a terminal row, silently resurrecting a completed, failed, or stopped apply. A resurrected apply re-enters driver claiming with no operator action behind it — the storage layer must refuse the transition regardless of which caller races.

What it does

  • Update refuses terminal-to-active transitions in the WHERE clause itself: when the new state is active, the write matches only rows that are still active, so a concurrent terminal verdict can never be overwritten from a snapshot.
  • The ambiguous zero-rows result (idempotent no-op vs guarded refusal vs missing row) is resolved by a read-committed re-read: a terminal row surfaces the distinct ErrApplyTerminalStateImmutable, a missing row surfaces ErrApplyNotFound, and a still-active row is a benign no-op.
  • Terminal-to-terminal writes, including same-state refreshes, stay allowed. A settled apply re-enters the active lifecycle only through the dedicated guarded transition of claiming a stopped apply.
  • The stop-before-start normalization in the API layer reloads the row when its write is refused and proceeds from the newer verdict instead of overwriting it; guard errors carry the apply identifier, not the internal row ID.
caller snapshot (Running)          concurrent driver
        |                                 |
        |                          write Failed (terminal)
   Update(state=Running) ---------------> WHERE state IN (active)
        |                                 -> 0 rows
   re-read row: terminal
        -> ErrApplyTerminalStateImmutable (never resurrected)

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings July 2, 2026 04:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enforces “terminal apply state immutability” as a storage-layer safety invariant, preventing stale writers from moving an apply from a terminal verdict back into the active lifecycle, and hardens the stop-before-start normalization path with a CAS write to avoid clobbering concurrent state advances.

Changes:

  • Add a storage guard in Applies().Update that refuses terminal → active transitions with a distinct storage.ErrApplyTerminalStateImmutable.
  • Disambiguate “0 rows affected” outcomes for guarded active-state updates by re-reading the row to distinguish missing vs terminal vs idempotent no-op.
  • Update the stop-before-start handler to use UpdateDerivedState (CAS) and add coverage for the concurrent-advance scenario.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/storage/storage.go Documents new Update() behavior around terminal immutability.
pkg/storage/mysqlstore/applies.go Implements terminal→active guard + zero-rows disambiguation helper.
pkg/storage/mysqlstore/applies_test.go Adds/updates tests for terminal immutability and Update() missing-row behavior.
pkg/storage/errors.go Introduces ErrApplyTerminalStateImmutable.
pkg/api/handlers_test.go Extends staticApplyStore test double with UpdateDerivedState CAS semantics.
pkg/api/control_handlers.go Uses UpdateDerivedState to CAS the stop-before-start normalization write.
pkg/api/control_handlers_test.go Adds tests covering the CAS stop-before-start behavior and concurrent advance handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/storage/storage.go
Comment thread pkg/storage/mysqlstore/applies.go Outdated
Comment thread pkg/storage/mysqlstore/applies.go Outdated
Comment thread pkg/storage/mysqlstore/applies.go Outdated
@aparajon
aparajon force-pushed the armand/terminal-state-guard branch from 739c1cd to 68a6f3d Compare July 18, 2026 17:25
@aparajon
aparajon force-pushed the armand/terminal-state-guard branch from 68a6f3d to c7aac95 Compare August 7, 2026 14:32
@aparajon aparajon changed the title fix(storage): terminal apply states are immutable outside explicit reapply fix(storage): terminal apply states are immutable Aug 7, 2026
Applies().Update was last-writer-wins: a caller holding a stale in-memory
snapshot could write an active state over a terminal row, resurrecting a
completed, failed, or stopped apply. Update now refuses terminal-to-active
transitions in the WHERE clause and surfaces the refusal as a distinct
ErrApplyTerminalStateImmutable, resolved from the ambiguous zero-rows
result by a read-committed re-read of the row.

Terminal-to-terminal writes (including same-state refreshes) stay allowed;
a settled apply re-enters the active lifecycle only through the dedicated
guarded transition of claiming a stopped apply. The stop-before-start
normalization in the API layer now reloads the row and proceeds from the
newer verdict instead of overwriting it, and guard errors surface the
apply identifier rather than the internal row ID.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aparajon
aparajon force-pushed the armand/terminal-state-guard branch from c7aac95 to 07eccbe Compare August 7, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants