fix(operator): pace the automatic retries of an interrupted apply, and skip them when the target's verdict is final - #1041
fix(operator): pace the automatic retries of an interrupted apply, and skip them when the target's verdict is final#1041aparajon wants to merge 10 commits into
Conversation
A failure that reproduces instantly — a refused lock, a connection error — spent the whole recovery budget in well under a minute, because nothing separated one automatic attempt from the next. The faster a failure failed, the less real recovery time its budget bought, on exactly the failures a little time was most likely to clear. Each admitted attempt now arms a wait for the next one on the apply's new retry_after column, and the failed_retryable claim clause honours it. The wait is measured from the start of an attempt, so an attempt that ran longer than its own backoff retries as soon as it fails. The first couple of retries stay immediate and the wait then steps up and holds flat, spreading a fully spent budget over roughly ten minutes. Operator starts and stale-lease recovery are unchanged: neither waits out a retry nobody asked for. The retry line in the PR comment and the CLI progress box now name the attempt and the clock time the next one is due, so a waiting apply reads apart from a stalled one. **`orders`**: 🟧🟧🟧⬜… 🔄 Retrying · attempt 3/10 · next 14:32 UTC
cdaa73b to
f75d8dc
Compare
aparajon
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces paced automatic retries for failed_retryable applies by adding a durable applies.retry_after gate that the claim logic honors, preventing fast-reproducing transient failures from burning the full retry budget immediately. It also surfaces the current attempt and the next eligible retry time in the PR comment preview/templates and the CLI progress view.
Changes:
- Add
retry_afterstorage/schema support plus a capped backoff policy (storage.RetryBackoff) and enforce it in retryable-claim SQL predicates. - Render “Retrying · attempt N/10 · next HH:MM UTC” in PR comments and expose attempt/retry timing in the progress API/CLI.
- Add unit/integration coverage for the backoff policy and claim gating, plus lifecycle documentation updates.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| TEMPLATES.md | Updates the example interrupted/retrying table row output to include attempt + next eligible time. |
| pkg/webhook/templates/preview.go | Updates preview data to include attempt and computed RetryAfter for deterministic template rendering. |
| pkg/webhook/templates/apply.go | Adds RetryAfter to template data and renders the “next HH:MM UTC” segment when applicable. |
| pkg/webhook/templates/apply_test.go | Updates retryable rendering assertions and adds coverage for the “next retry” segment behavior. |
| pkg/webhook/multi_apply.go | Plumbs stored RetryAfter into multi-deployment comment data. |
| pkg/webhook/apply.go | Plumbs stored RetryAfter into single-deployment comment data. |
| pkg/storage/types.go | Introduces capped backoff constants and RetryBackoff, plus Apply.RetryAfter field. |
| pkg/storage/types_test.go | Adds tests validating the backoff step/cap and overall retry pacing window. |
| pkg/storage/internal/sqlstore/apply_operations.go | Gates retryable operation claims on retry_after and arms backoff on redispatch. |
| pkg/storage/internal/sqlstore/applies.go | Adds retry_after column scanning, claim gating predicate, and arms backoff on retryable claims. |
| pkg/storage/internal/sqlstore/applies_test.go | Adds integration tests proving retry backoff gating and exceptions (operator start, stale lease recovery). |
| pkg/schema/postgres/applies.sql | Adds retry_after column to Postgres applies table. |
| pkg/schema/mysql/applies.sql | Adds retry_after column to MySQL applies table. |
| pkg/cmd/internal/templates/progress.go | Adds a “Retry” detail row in CLI progress output with attempt and next eligible time. |
| pkg/cmd/internal/templates/progress_parse.go | Adds Attempt and RetryAfter fields to parsed progress data. |
| pkg/apitypes/apitypes.go | Extends ProgressResponse to include attempt and retry_after. |
| pkg/api/progress_handlers.go | Overlays attempt/retry timing from stored apply state onto progress responses. |
| pkg/api/ensure_schema_postgres_test.go | Updates schema column expectations for the new Postgres retry_after column. |
| docs/apply-lifecycle.md | Documents paced retries, what’s gated vs not gated, and how it’s surfaced to operators/authors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Three follow-ups on the automatic-retry pacing. Store the deadline at the precision the policy is written in. The deadline expression and the predicate that reads it back are both microsecond-precision, but the MySQL column rounded to whole seconds, so a sub-second step in the policy would shift the wait by up to a second — in the direction that holds back a retry carrying no wait at all — and the shift would depend on which dialect the deployment runs. The column now matches Postgres, and the comparison is made at the same precision as the value. Report the deadline only while the apply is waiting on it. A claim leaves the armed deadline behind on the row it resumes, so a running or completed apply carried a time naming a retry that was not coming. The spent attempt count still reports in every state: on a permanently failed apply it is the record of how many recoveries were tried. Render the CLI retry row against the package's nowFunc rather than time.Now, so a preview with a pinned clock renders deterministically like every other time-dependent row in that view.
The retry row had no preview fixture, so the one view that tells an operator an apply is waiting rather than wedged could not be seen without reproducing an interruption. Adds a `retrying` progress preview: budget spent, next attempt due, mid-copy table, and the cause. Its next-attempt time is derived from the real backoff policy, so the preview cannot drift from what an operator will see. Two things the fixture surfaced. Absolute clock times rendered in the zone of whoever ran the generator, which would have made TEMPLATES.md churn between machines. The zone is now a package variable that preview mode pins to UTC, matching how the package already pins its clock. The failure cause was printed under the box for a permanently failed apply but not an interrupted one, so a retrying apply named its next attempt with no hint of what it was retrying past. Both states ask the operator the same question, and only the message answers it.
…face The PR comment drew a retrying table's bar orange while the CLI drew it yellow, so the same apply read differently depending on where an operator looked. Yellow in this vocabulary is a healthy wait — waiting for cutover, revert window open — and an apply that keeps failing while it spends a finite budget toward permanent failure is not that. Orange is the halted family: work stopped partway, not progressing until something resumes it, which is exactly an apply between automatic retries. Both surfaces now use it, and the CLI's state label and status colors follow their own bar, as they already do for every other state.
|
🤖 Review findings - created by Kiran's code review agent - for pull/1041, dc2269c. Verdict: 8 findings — 4 non-blocking, 4 general suggestions, nothing blocking; CI is green with 34/34 checks passing at head dc2269c (Build, Unit, Integration x2, all E2E suites incl. K8s/Vitess/gRPC multi-deployment, LocalScale x3, all lint jobs, DCO). Non-blocking
General suggestions
The one thing that could have broken, verifiedThe riskiest mechanism is the new Verified correct
This review was generated by Claude Code (claude-fable-5). |
The claim path admits an automatic retry only while attempt is under MaxRecoveryAttempts, but both operator-facing surfaces announced attempt+1 unconditionally. An apply whose last attempt failed inside its backoff therefore read "attempt 11/10 · next HH:MM UTC" — naming a retry no driver will ever claim, on a comment that is not re-rendered again before expiry terminalizes it. AnnouncedRetryAttempt puts the ceiling next to the budget constant it belongs to, so the PR comment and the CLI progress view agree: a spent budget names the last attempt made and promises no time.
The operation claim path gates on the parent apply's retry_after and arms the next wait on it, but only the single-apply path had tests. A regression there would silently restore unpaced budget burn for exactly the fan-out applies the pacing is meant to protect. Both directions are now pinned: a failed_retryable operation stays unclaimable until the parent's wait elapses, and an admitted redispatch arms the parent's next wait while consuming one unit of the shared budget. The concurrency note alongside says how far the concurrent-claim window actually extends now that the armed wait gates siblings too.
A deployment row of a multi-deployment apply rendered its own operation state against the parent's armed deadline. An operation is re-leased for redispatch only while the parent apply is itself failed_retryable, so a failed deployment of a still-running rollout named a time nothing was going to act on. The preview fixtures were arming a wait one policy step ahead of the attempt they depict; they now use the wait the real policy arms for that attempt, and the retrying preview's listing says it shows the next-attempt time.
|
🤖 Thanks — all eight addressed across c8de8e9, 1dace5b, and b64ea57. Findings 1, 3, 4, 5, 6, 7 and 8 are fixed as described. On 1, the overrun is now capped in Finding 2 is now a "Before deploying" section in the PR body, including the note that a deployment taking this and #1039 together needs both columns first. — Claude Code (Opus 5) |
… retrying it A schema change the target rejects for a reason that belongs to the statement or to the data already in the table reproduces on every attempt. Reporting it as retryable spent the whole recovery budget re-running it, and held the database's active-apply slot for the entire wait. The engine now classifies its failures: duplicate values under a new unique index, rows that do not fit a narrowed column, a column the table does not have, DDL the target refuses outright, and the checksum failure that is how a unique index over non-unique data fails, are reported as permanent. Everything the target might answer differently once a lock, a lagging replica, or a busy target has moved on keeps its retries.
aparajon
changed the title
The comment claimed each attempt paid for another full table copy. Spirit resumes past a completed copy from the persisted copier watermark, so the copy is not redone. What is redone is the verification: a checksum that found differences is not allowed to persist a watermark, so every attempt restarts the checksum phase from the beginning. Still a whole-table read per attempt, and still entirely wasted on a rejection that reproduces, but name the cost accurately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Why this matters
A transient failure moves an apply to
failed_retryableand a recovery driver redispatches it, up to a fixed budget of attempts. Nothing separated one attempt from the next, so the budget was spent fastest by the failures that reproduce fastest: a refused lock or a connection error could burn all ten attempts in well under a minute — on exactly the failures a little time was most likely to clear. The apply reached permanentfailedbefore the condition it was waiting on had any chance to lift, and the operator sees a dead apply rather than one still trying.What it does
Each admitted attempt arms a wait for the next one on a new
applies.retry_aftercolumn, and thefailed_retryableclaim clause honours it. The wait steps up and then holds flat, so the first couple of retries stay immediate for a blip while a fully spent budget spreads over roughly ten minutes of wall clock instead of a minute.The wait is armed when an attempt is admitted, not when it fails, so it is measured from the start of that attempt: an attempt that ran longer than its own backoff has already spaced itself out and retries the instant it fails. Only the automatic-retry clause is gated — an operator
startruns now, and a driver that dies mid-attempt is picked up by a peer as soon as its lease goes stale. Neither waits out a retry nobody asked for.The deadline is computed by the database rather than bound as a Go timestamp, so the wait never depends on the app and database clocks agreeing, and it is stored at the precision the policy is written in on both dialects — a coarser column would shift the wait, in the direction that holds back a retry carrying no wait at all.
The PR comment and the CLI progress box name the attempt and the clock time the next one is due, so a waiting apply reads apart from a stalled one. The time is absolute rather than a countdown: the comment is not re-rendered while an apply waits, and a countdown would freeze at whatever it read when posted. It is reported only while the apply is actually waiting — a claim leaves the armed deadline behind on the row it resumes, and a resumed apply must not advertise a retry that is not coming.
Two consistency fixes around that surface: an interrupted apply now draws in the halted color on every surface (the CLI drew it in the healthy-wait yellow while the PR comment drew it orange), and the CLI prints the failure cause for an interrupted apply as well as a permanently failed one — naming the next attempt without saying what it is retrying past leaves the operator nothing to judge. The interrupted progress view gets a preview fixture, so both are visible in
TEMPLATES.md.Failures that are not worth pacing
Pacing only helps a failure that time can clear. The other kind reproduces on every attempt, and spacing those out is strictly worse: the same verdict arrives ten times over ten minutes while the apply holds the database's active-apply slot, and the operator watches a countdown to a foregone conclusion.
Those failures now skip
failed_retryableentirely. The engine classifies what the target told it: a duplicate value under a new unique index, existing rows that do not fit a narrowed column, a column the table does not have, DDL the target refuses outright. A unique index over non-unique data is the case worth naming — the row copy drops the duplicates rather than refusing them, so the rejection never arrives as a target error at all; it surfaces as a checksum that can never be made to agree, after a full copy. Retrying that spends the budget on ten full table copies.Membership in the permanent set is deliberately narrow, and the asymmetry decides the edge cases: a rejection left out keeps the retries and costs the operator only the wait they were already spending, while one wrongly added ends an apply that would have recovered on its own. Anything the target might answer differently once a lock, a lagging replica, disk, or a peer transaction has moved on — lock wait timeouts, deadlocks, read-only, connection loss — keeps its retries.
How it moves us toward the northstar
Unattended operation means the automatic paths have to be worth trusting. A retry budget that is really a budget of reproductions gives up on the transient failure it exists for, and turns a self-healing apply into an operator escalation. Pacing the attempts makes the budget a span of recovery time, spending it only where time is what is missing, and naming the next attempt in the PR comment lets an author tell "waiting" from "wedged" without asking anyone.
Before deploying
applies.retry_afteris new in both dialect schema files. On MySQL the startup diff adds it automatically. The PostgreSQL bootstrapper verifies that expected columns exist but does not alter existing tables, so a database already bootstrapped by an earlier version needs the column added before this one starts:A deployment taking this change and #1039 together needs both PRs' columns in place first.
Opened by Claude (Opus 5).