fix(e2e): accept the stored attempt counter as proof of a retryable pause

[Kiran01bm]

Deflakes the k8s retryable-pause e2e by accepting the data plane's stored attempt counter, alongside the wire state, as proof that the injected pause happened.

Why

TestK8s_DataPlaneRetryablePauseHoldsControlPlaneOpenUntilRecovery kills the data plane's target connections every 250 ms until it sees STATE_FAILED_RETRYABLE on the wire, then stops injecting and waits for recovery. The pause is transient by design: a driver's claim poll reclaims a failed_retryable apply as soon as it sees it, and with several drivers polling there is no cooldown, so the wire can show the pause for less than one observation tick. When the test misses it, every subsequent kill lands on the recovery attempt it is supposed to be waiting for, and the loop times out after three minutes with the wire still reporting running.

What

Every write that advances the data plane's applies.attempt column is gated on the apply being in failed_retryable: the claim transition out of it, and the sibling-operation redispatch under the same guard, each increment atomically inside their own transaction; the create-time insert seeds it. An increase past its pre-kill value is therefore durable proof that a retryable pause occurred and recovery has already claimed it. The poll now ends when either the wire shows the pause or the stored counter has advanced. The per-tick assertions are unchanged: the control plane must stay non-terminal, the wire must never settle failed, and at least one kill must have landed. The 3-minute budget and 250 ms interval are unchanged.

Before / after

Before                                   After

each tick:                               each tick:
  assert control plane non-terminal        assert control plane non-terminal
  wire == FAILED_RETRYABLE? ── yes ─▶ stop  wire == FAILED_RETRYABLE? ── yes ─▶ stop
        │ no                                     │ no
        ▼                                        ▼
  kill target connections                  stored attempt > baseline? ─ yes ─▶ stop
                                                 │ no
                                                 ▼
pause reclaimed between ticks:             kill target connections
  wire never shows it, kills keep
  hitting the recovery run, timeout      pause reclaimed between ticks:
                                           attempt advanced, injection stops,
                                           recovery completes

[chatgpt-codex-connector]

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

[Copilot]

🟢 Approval recommended

The change is confined to E2E test logic and uses an authoritative storage-side signal to eliminate a known transient-wire-state flake without weakening existing assertions.

Pull request overview

This PR deflakes the Kubernetes E2E that verifies the data plane’s retryable pause behavior by accepting a durable storage-side signal (the data plane apply’s applies.attempt counter) as proof the pause occurred, even if the transient STATE_FAILED_RETRYABLE wire state is missed between polling ticks.

Changes:

• Extends the test’s polling stop condition to succeed when either the wire reports STATE_FAILED_RETRYABLE or the stored attempt counter advances past its pre-injection baseline.
• Adds a small helper to read the data plane apply’s stored attempt value and improves the timeout message to include both wire and attempt observations.

File summaries

File	Description
e2e/k8s/retryable_pause_test.go	Updates the retryable-pause observation loop to treat an incremented stored attempt counter as durable evidence the retryable pause occurred, reducing flakiness when the wire state is too brief to observe.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0
• Review effort level: Lite

---

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

[aparajon]

🤖 Adversarial correctness review — 085113b0 (1 file, +39/−8)

The diagnosis is right and the fix is the right shape: a reclaim with no cooldown can consume the pause faster than a 250 ms observation tick, so waiting on the wire alone is a race the test can't win, and a durable counter is the correct second witness. Observing before injecting, and keeping require.Positive(t, kills) (:111), are what keep the new exit from becoming a free pass.

Everything here rests on one claim, so that's what I went after: does an advance in applies.attempt really prove the apply was in failed_retryable? If any other path can raise that counter, the new exit condition (:99) fires without a pause and this test — whose whole job is proving the control plane doesn't go terminal on a live remote apply — silently passes on nothing.

It holds, but not for the reason the PR states. A full census of the writers (11 UPDATE applies statements in pkg/, plus both INSERT INTO applies sites):

applies.go:2089    claim transition   attempt = CASE WHEN ? = ? THEN attempt + 1 ELSE attempt END
                                      params: apply.State, state.Apply.FailedRetryable
                                      → increments only from failed_retryable          ✓

apply_operations.go:1431  operation redispatch   UPDATE applies SET attempt = attempt + 1
                                      WHERE id = ? AND state = ?          ← state.Apply.FailedRetryable
                                        AND attempt < ?                   ← maxRecoveryAttempts
                                        AND EXISTS (sibling operation)    ← multi-op only
                                      → also requires the PARENT to be failed_retryable  ✓

applies.go:566, :744      INSERT            sets attempt at create

Two increment paths, not one — but both require applies.state = failed_retryable, so the inference the test draws is sound either way. No reset path exists (no other UPDATE applies touches the column). That's the property worth writing down, and finding 2 is that the comment writes down something narrower.

#	Sev	Where	What
1	low	:24 vs :33-39	The header now contradicts itself — bullet 1 still asserts the pause "crosses the wire as STATE_FAILED_RETRYABLE" as a proven property, which the new paragraph directly retracts
2	low	:147, PR body	"advances only when a driver claims the apply out of failed_retryable" misses the operation-redispatch bump, and names a mechanism where the guarantee is a property of every writer

---

1 — the header asserts what the test no longer proves (low)

The bullet list at :24-31 still opens with:

//   - The pause crosses the wire as STATE_FAILED_RETRYABLE, so the control
//     plane can tell "paused, will self-retry" from a settled failure without
//     inspecting per-table statuses.

and the paragraph added at :33-39 says the wire "may render STATE_FAILED_RETRYABLE for less than one observation interval" and that the counter is an accepted substitute. Both are in the same comment block, and they can't both describe this test: after this change a passing run may never have seen that wire state at all. A reader hits the bullet first and takes it as the contract.

This matters a little more than a stale-comment nit because the coverage really does go away. retryable_pause_test.go:95 is the only place in e2e/ that inspects the pod wire for STATE_FAILED_RETRYABLE — the e2e/grpc hits (helpers_test.go:360, multideploy_test.go:210,321,357) all read stored/API state, not the ternv1 wire enum. So if state propagation regressed such that the pause never rendered on the wire, this suite would now go green via the counter.

I don't think that argues against the change — the test is named for the control plane holding open, the pause-visibility bullet was always incidental to that, and a race you can't win isn't coverage. But the header should stop claiming it. Rewording bullet 1 to the capability ("the data plane distinguishes a pause from a settled failure, so the control plane can tell them apart without inspecting per-table statuses") keeps the point that matters and drops the assertion about what this test observes. If wire rendering of the pause is worth pinning on its own, it wants a test that can win that race — a stopped apply, or a fake, rather than a 250 ms poll against a live reclaim.

2 — the stated mechanism is narrower than the guarantee (low)

:147:

// The counter advances only when a driver claims the apply out of
// failed_retryable, so an increase is durable proof that a retryable pause
// happened and recovery has already picked it up

and the PR body's "written in exactly two places: the insert at create time and the claim transition out of failed_retryable". There's a third writer: apply_operations.go:1431 consumes the parent's retry budget when a driver re-claims a failed_retryable operation of a multi-operation apply. That one bumps applies.attempt without transitioning the parent out of failed_retryable at all — its own comment (:1382-1391) is explicit about why: without it "an operation-only multi-deployment retry never advances applies.attempt, so ExpireRetryable's budget path never fires and a permanently failing deployment retries forever."

The conclusion is unaffected: that path is gated on a.state = failed_retryable too, so an increase still proves the pause. And this test's apply looks single-operation, so the EXISTS (sibling) guard likely excludes the path here anyway. So there is nothing to fix in the code.

What's worth fixing is the shape of the claim, because the comment is the thing a future maintainer will trust. The guarantee the test depends on isn't "one specific transition writes this column" — it's "every path that advances applies.attempt requires the apply to be in failed_retryable". Stated that way, someone adding a fourth writer sees the constraint they have to preserve. Stated as it is now, they check whether they touched the claim transition, find they didn't, and this e2e quietly becomes a test that can pass without a pause. Same for the PR body, where "exactly two places" is a census a reviewer may take as verified.

---

Verified — the false-green guards, the reader that can't be killed, and why I couldn't mutation-test this one

require.Positive(t, kills) (:111) is what makes the new exit safe, and the tick ordering is load-bearing. The poll observes wire, then counter, then injects — so if the counter were already above the baseline on the first tick (a pause the test didn't cause), the poll returns true with kills == 0 and require.Positive fails rather than passing. A pre-existing or spontaneous retryable failure therefore surfaces as a failure, not a false green. The baseline at :67 is also read after waitForPodApplyState(..., STATE_RUNNING, ...), so it can't bake in a pause from earlier in the fixture.

The new reader cannot be killed by the injector, which I checked because it would have been a fresh flake. dataPlaneDB (:54) is storageDSNs(t)[0] — the tern staging server with DBName rewritten to tern — while killDataPlaneTargetConnections kills WHERE db = 'testapp' AND id <> CONNECTION_ID(). The reader's connections carry db = 'tern', so they're outside the filter. Had the helper reused TernStagingDSN directly this would have been a self-inflicted mid-poll require.NoError failure; [0] is the right handle, and it's the established idiom in this package (seven other storageDSNs(t)[0] call sites, e.g. k8s_test.go:329, operator_fixtures_test.go:67).

apply_identifier is UNIQUE KEY idx_apply_identifier (pkg/schema/mysql/applies.sql:32), so the helper's SELECT attempt FROM applies WHERE apply_identifier = ? with no LIMIT is single-row by construction rather than by luck.

The invariant the test now leans on is guarded by storage tests, so a regression fails CI instead of degrading this e2e into a tautology: applies_test.go:2375 and :2398 pin attempt unchanged when a retryable claim is refused (stale window, spent budget), and apply_operations_test.go:2115,2120,2159,2164 pin that a crash-recovery re-lease of a failed_retryable operation under a running parent leaves attempt untouched — which is precisely the "advance without a genuine redispatch" case that would break the inference. :1969 and :2002 pin the multi-op vs single-op budget accounting that finding 2 is about.

Invariants. This test is a guard for ST-9 (failed_retryable is active, not terminal; the budget is bounded) and for the orphaning half of RC-1 — its second bullet, that a terminal control-plane verdict "would end the drive and orphan a live remote apply", is RC-1's concern stated as a test. The change upholds both: the per-tick control-plane non-terminal assertion, the never-settle-failed assertion, and the recovery-to-completed assertion are all unchanged, and only the witness for entering the pause is widened. No *Enforced:* line moves — this is a test-only change.

One aside, clearly not this PR's business: ST-9's *Enforced:* line points at pkg/api/operator.go for budget semantics, but the increment itself and the attempt < maxRecoveryAttempts gate live in pkg/storage/internal/sqlstore/applies.go:2089 and apply_operations.go:1431. That's the "resolves cleanly to the wrong file" case AGENTS.md warns about, and I only noticed it because verifying this diff meant finding the real writers. Worth a separate look by whoever next touches that path.

Mutation testing: not possible for this one, and I'd rather say so than imply otherwise. e2e/k8s is //go:build e2e against a live cluster and staging databases, so I can't run the suite or mutate it into failing locally. What I did instead is the census above — an exhaustive enumeration of every writer to the column, which for a claim of the form "only X writes this" is stronger evidence than a mutation run would have been, since it covers paths a single test execution would never take. gofmt -l e2e/k8s/ clean and go vet -tags=e2e ./e2e/k8s/ clean at 085113b0. Helper uses t.Context() and require, per the testing conventions; the 3-minute budget and 250 ms interval are untouched, so no deadline was widened to buy the deflake — which is the thing I check first on a flake fix.

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

[aparajon]

🤖 Approving. The deflake is correctly diagnosed and the second witness is the right one: an advance in applies.attempt really does prove the apply passed through failed_retryable, and require.Positive(t, kills) keeps a pause the test didn't cause from becoming a false green. Two low findings in the review comment, both about comments rather than code — the header still asserts the wire property the change retracts, and the "advances only when a driver claims the apply out of failed_retryable" claim misses a third writer (the operation-redispatch bump in apply_operations.go) whose gate happens to preserve the conclusion. Upholds ST-9 and RC-1; test-only, no *Enforced:* line moves.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

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

Four of five findings on #1335 are fixed in the follow-up commit (plus a PR-body correction); the apply-log witness and the e2e lint gap are out of scope for this PR.

#	Finding	Status	Explanation
2	Docstring bullet 1 promises the wire shows STATE_FAILED_RETRYABLE, but the loop can exit on the storage witness	fixed	Bullet now states what the test proves: the pause never settles as STATE_FAILED; the wire either renders the pause or has already moved on to recovery.
4	Completion poll opens a fresh *sql.DB per tick against the DSN dataPlaneDB already holds; new helper's signature differs from neighbours	fixed	Split a handle-taking storedApplyAndTaskStates(t, db, id) out of storedK8sApplyAndTaskStates, which now opens and delegates; the poll reuses dataPlaneDB. Existing DSN-taking callers unchanged.
3	"advances only when a driver claims…" and the PR body's "exactly two places" miss the sibling-operation redispatch write	fixed	Docstring now reads "only advances out of failed_retryable"; PR body corrected to say every advancing write is gated on that state.
5a	New SQL does not backtick-quote identifiers	fixed	applies, state, attempt, apply_identifier quoted in the file's single-line queries.
5b	make lint never covers //go:build e2e files	rejected	Real gap, but repo-wide and independent of this PR; belongs in its own change to .golangci.yaml / make lint. This follow-up was linted with --build-tags e2e by hand.
1	Attempt counter latches at the recovery claim; the append-only apply-log entry would close the residual kill window	rejected	Agreed on the mechanism, and the review itself scopes it as a follow-up. The counter is kept as the witness here because its write cannot be lost, whereas the log append is best-effort; the log witness would be an additional, earlier signal, not a replacement.

"The one thing that could have broken, verified" (buffered ticker explains the sub-tick pause) and the "Verified correct" section: no action.

Source: adversarial review of #1335 at 085113b, generated by Kiran's code review agent (Claude Code / claude-opus-5); not posted on the PR.