fix(appplatform): retry transient referential-integrity and server errors

[rknightion]

Summary

App Platform resources only retried HTTP 409 conflicts on delete, and did not retry reads at all. Against the provisioning (Git Sync) API this surfaces as two intermittent, eventual-consistency failures:

1. Destroy 422 — cross-resource referential-integrity race. When a connection and a repository that references it (spec.connection.name) are destroyed together, Terraform deletes the repository first. The repository DELETE returns 200 and stamps a deletionTimestamp, but the object lingers in storage until its finalizers complete.

   The connection delete validator decides at admission via a live ListByConnection (field selector spec.connection.name). With fix(provisioning): ignore terminating repositories when validating connection delete grafana#126822 (merged) the validator now ignores repositories that are themselves terminating (DeletionTimestamp != nil), which reduces the previously-persistent 422 to a narrow storage read-after-write window: if the validator's list has not yet observed the repository's freshly-written deletionTimestamp (~100ms; a field-selector list may be served from the eventual-consistent index path rather than a quorum read), it still counts the repository as a live reference and returns:

   Error: invalid value for field "metadata.name":
     * Forbidden: cannot delete connection: referenced by 1 repository(s): [...]
   

   This 422 (a StatusReasonInvalid, not a 409) was not retried, so the destroy failed even though the window clears moments later. The backend change and this client retry are complementary halves of the fix.

2. Plan/refresh 5xx. A transient 500 (or 503/timeout/429) on a GET during plan/refresh failed the whole operation with no retry. No server-side change covers this yet, so it stands on its own.

Changes

• Extract a generic retryWhile(ctx, attempts, delay, retryable, run) helper. retryOnConflict is now a thin wrapper over it — update-path behaviour is unchanged (still 409-only, 5×200ms). A non-positive attempt budget is clamped to a single attempt, so a misconfigured budget can never return nil without running.
• Delete now retries on conflict (409) or a transient referential-integrity error (422 Invalid whose message reports it is still "referenced by" another resource), bounded at 6×2s so a genuinely-permanent reference still surfaces its real error promptly rather than hanging.
• Read and ImportState now retry transient server errors (500/503/server-timeout/504/429) at 5×1s; NotFound is unaffected and still removes the resource from state.
• Predicates (isReferencedDependencyError, isRetryableDeleteError, isRetryableServerError) and retryWhile are unit-tested, including negative cases (a genuine validation 422 is not retried) and the non-positive-attempts guard.

Scope / blast radius

The retry logic lives in the generic App Platform Resource[T, L] base, so it applies to all App Platform resources, but the new behaviour only changes outcomes for the transient error classes above; non-retryable errors fail exactly as before. The referential-integrity 422 is the documented behaviour of the provisioning connection delete validator (apps/provisioning/pkg/connection/delete_validator.go).

Related backend work

• fix(provisioning): ignore terminating repositories when validating connection delete grafana#126822 (merged): delete validator ignores terminating repositories — turns the persistent 422 (entire finalizer-completion window, seconds) into a transient one (storage read-after-write lag) that this PR absorbs.
• fix(provisioning): gate connection deletion with a referenced-only finalizer grafana#126819 (closed): a finalizer-based alternative; explored but closed. It doesn't touch the validator's read path, so it would not have removed the need for this retry.

Notes / follow-ups

• The "referenced by" classification matches the admission controller's message text (gated behind IsInvalid); the marker is centralized in a named constant with a source pointer. The 422 is a generic Invalid + Forbidden on metadata.name, so the message text is the only distinguishing signal available client-side — a stable machine-readable marker would be a backend follow-up.
• The update-path's inner Get (on conflict re-fetch) is intentionally left as conflict-only retry — pre-existing behaviour, out of scope for this fix.
• The combined connection+repository destroy path is exercised by the existing TestAccProvisioningRepository_viaConnection acceptance test.

Testing

• go test ./internal/resources/appplatform/... — pass
• go vet ./internal/resources/appplatform/... — clean
• golangci-lint run ./internal/resources/appplatform/... — 0 issues
• gofmt — clean

No schema/example changes, so no docs regeneration required.

[github-actions]

In order to lower resource usage and have a faster runtime, PRs will not run Cloud tests automatically.
A Grafana Labs employee can trigger them by commenting /run-cloud-tests on this PR, or via the Actions UI for the fix/provisioning-delete-read-retries branch.

[rknightion]

/run-cloud-tests

[github-actions]

Cloud acceptance tests dispatched for fix/provisioning-delete-read-retries: view run

[Copilot]

⚠️ Not ready to approve

retryWhile can return nil without executing the operation when attempts is 0, which can mask real errors and should be guarded against.

Pull request overview

This PR hardens App Platform resource CRUD operations against eventual-consistency and transient backend failures by introducing a generic retry helper and expanding retry conditions for reads and deletes in the shared Resource[T, L] base.

Changes:

• Adds a generic retryWhile helper and refactors retryOnConflict to wrap it.
• Extends Delete retries to include transient referential-integrity 422 Invalid errors (and keeps retrying 409 conflicts) with a longer bounded budget.
• Adds Read/ImportState retries for transient server-side errors (5xx/timeouts/429) to reduce flaky plan/refresh failures.

File summaries

File	Description
internal/resources/appplatform/resource.go	Introduces retryWhile + retry predicates; applies retry to Read/ImportState and broadens Delete retry behavior.
internal/resources/appplatform/resource_test.go	Adds unit tests covering the new retry helper and retryability predicates (positive + negative cases).

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 1

Note

Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.

---

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

[MissingRoberto]

I think this should be solved in Grafana directly, and not the provider. If you received a 200 for the deletion of the repository, connection should not fail.

[MissingRoberto]

Backend context for this PR, now that the server-side change has landed.

The 422-on-delete retry here is a required half of the fix — please keep it.

On the backend, grafana/grafana#126822 changed the connection delete validator to ignore repositories that are themselves terminating (DeletionTimestamp != nil). That addresses the root cause of the terraform destroy failure:

422 Forbidden: cannot delete connection: referenced by N repository(s)

But it does not fully eliminate the 422 — it shrinks it to a storage read-after-write window. The validator decides at admission by doing a live ListByConnection (field selector spec.connection.name):

1. repository DELETE → 200 (deletionTimestamp committed)
2. ~100ms later, connection DELETE → validator lists repositories

If that list has not yet observed the repository's freshly-written deletionTimestamp (plausible — a field-selector list is likely served from the eventual-consistent search/index path rather than a quorum read), the validator still counts the repository as live and returns the same 422. Without a client retry, that transient 422 fails the run.

So the two changes are complementary and both are needed:

• Backend (#126822): turns a persistent 422 (entire finalizer-completion window, seconds) into a transient one (storage-lag window).
• This PR: retries that transient 422 so the module succeeds.

Separately, the read-retry for transient 5xx (the intermittent 500-on-GET during plan/refresh) in this PR addresses a distinct backend issue that no server-side change covers yet, so that part stands on its own.

For the record: a backend finalizer approach was explored (grafana/grafana#126819) but closed — it doesn't touch the validator's read path, so it would not have removed the need for this retry.

[MissingRoberto]

a couple minor comments about the delay values

[ferruvich]

Overall LGTM - I agree on @MissingRoberto's comment about durations we set for delays

[rknightion]

Keeping the 422 retry. Updated the deleteRetry* / isReferencedDependencyError comments and the PR description to the post-#126822 model (residual 422 = storage read-after-write window on the validator's live ListByConnection, not a finalizer wait), and linked #126822 / #126819. 5xx read-retry stands on its own as you said.

[MissingRoberto]

Tuned the retry counts to match the new delays (pushed in 387b46f): delete is now 4 × 2s (~6s wall) and read 3 × 1s (~2s wall), keeping the total wait budgets bounded so a genuinely-permanent reference still surfaces its real error promptly.

Follow-up: these flat attempts × delay budgets are a reasonable first cut, but they're still a guess about the reconcile / read-after-write window. We'll open a follow-up to move this to exponential backoff (catch the common sub-second clear fast, grow the delay to absorb a slow window, ideally bounded by elapsed wall-time rather than a fixed attempt count). Keeping it flat here to keep this fix focused.

[MissingRoberto]

@rknightion you commits must be verified :(

[rknightion]

@rknightion you commits must be verified :(

sorry (first time comiting to a grafana repo). Signed now and rebased (worth noting for #2844 so you can rebase and avoid reintroducing my unsigned commits)