vendor-charts: index pre-check classifies retryable failures but never retries them

[yuanchen8911]

Summary

defaultFetchIndexYAML classifies retryable failures and then never retries them. A single transport reset while fetching a Helm repository index hard-fails aicr bundle — and any CI job that exercises it — regardless of what the run was actually changing.

Observed on PR #2244, a documentation-only change with zero Go files touched:

[cli] command failed: error=[SERVICE_UNAVAILABLE] vendor-charts: index pre-check
GET https://prometheus-community.github.io/helm-charts/index.yaml failed:
read tcp 10.1.0.205:49616->185.199.111.153:443: read: connection reset by peer
--- FAIL: chainsaw/cli-bundle-vendor-charts (3.18s)

Re-running the same commit with no changes passed. The endpoint was healthy throughout (HTTP 200, 6,243,154 bytes), and the same test had passed on the three previous heads of that branch.

The defect

pkg/bundler/deployer/localformat/vendor.go (defaultFetchIndexYAML, ~L585-660) splits failures into structured codes specifically so a caller can decide retryability. Its own comments say so:

// Split HTTP status into the closest structured code so
// orchestrators branching on the returned code get the right
// retryability signal:
//   - 404             -> NotFound          (chart repo does not exist)
//   - 401 / 403       -> Unauthorized      (caller must fix creds; retry same request won't help)
//   - 408 / 429       -> Unavailable       (retryable — request timeout / rate-limited)
//   - other 4xx       -> InvalidRequest    (caller-shaped problem)
//   - 5xx / everything else -> Unavailable (transient upstream)

and on the transport path:

// Anything else is a genuine transport/dial failure → Unavailable.
return nil, errors.PropagateOrWrap(err, errors.ErrCodeUnavailable, ...)

No caller retries on Unavailable. The classification is correct and unused, so every transient failure is terminal.

Why it is worth fixing

• The fetch is large and remote: the prometheus-community index is ~6 MB over the public internet from GitHub Pages. Long transfers carry real reset probability.
• It is a pre-check, not the payload. Failing an entire bundle run on a validation fetch is disproportionate to its role.
• It is not only CI. Operators running aicr bundle against an upstream Helm repository get a one-shot attempt bounded by HelmChartIndexPreCheckTimeout with no retry.
• CI red from this carries no signal about the change under test, which trains reviewers to re-run on red rather than read the log.

Proposed fix

Follow the pattern already established in pkg/oci/push.go (~L1280-1327) rather than introducing a second retry idiom. That loop already does the right things: bounded attempts, exponential backoff with jitter, a per-attempt timeout distinct from the parent context, a context-aware sleep that cannot outlive cancellation, an immediate return for non-transient errors, and a slog.Warn per retry so flakes stay visible.

Applied here:

1. Wrap the client.Do call and its status classification in a bounded retry loop.
2. Retry only ErrCodeUnavailable: transport/dial failures, 5xx, 408, 429.
3. Never retry ErrCodeNotFound, ErrCodeUnauthorized, or ErrCodeInvalidRequest.
4. Add HelmChartIndexRetryBudget and HelmChartIndexRetryInitialBackoff to pkg/defaults, mirroring SigstoreRetryBudget (3) and SigstoreRetryInitialBackoff (1s).
5. Honor context cancellation during backoff; never sleep past the parent deadline.
6. slog.Warn each retry with attempt number and error.

Security constraint on the retry predicate

The egress-policy redirect rejection is deliberately surfaced as InvalidRequest, and the existing comment explains why:

// A policy-rejected redirect surfaces here as a wrapped
// StructuredError with InvalidRequest — preserve its code so
// callers can distinguish "someone tried to smuggle us into a
// private range" from "upstream network is flaky."

The retry predicate must therefore key on the structured code, not on "the call returned an error." Retrying a policy-rejected redirect would repeatedly re-attempt an SSRF target and turn a fail-closed control into a retry loop. This is the one case where a naive if err != nil { retry } would be a security regression rather than a robustness improvement.

Note on errors.IsTransient

errors.IsTransient (pkg/errors/errors.go:142) currently reports true only for context.DeadlineExceeded, context.Canceled, and ErrCodeTimeout — not ErrCodeUnavailable. So this needs either an explicit Unavailable check at this call site, or a deliberate widening of the shared helper.

Prefer the call-site check. Widening IsTransient changes retry behavior everywhere it is consumed (including pkg/oci/push.go), which is a larger blast radius than this issue justifies. If the inconsistency is worth resolving, it should be its own change with its own review.

Testing

vendor_test.go already injects fetchIndexYAML, so no new seam is required. Drive a fake transport and assert attempt counts per class:

• reset / 503 / 429 / 408 → retried up to the budget, succeeds if a later attempt succeeds;
• 404 / 401 / 403 → exactly one attempt;
• egress-policy-rejected redirect → exactly one attempt (the regression guard for the security constraint above);
• parent context canceled mid-backoff → returns promptly, no further attempts.

Acceptance criteria

• A transient index-fetch failure no longer fails the run when a retry succeeds.
• Non-transient failures (404, 401/403, other 4xx) still fail on the first attempt, with their existing structured codes unchanged.
• A policy-rejected redirect is never retried.
• Retry budget and backoff are named constants in pkg/defaults, not literals.
• Backoff honors context cancellation.
• Each retry emits a slog.Warn so flakes are observable rather than silent.
• Tests pin attempt counts for every class above.

Out of scope

Caching the index between runs. It would also reduce exposure, but it is a different change with its own staleness and invalidation questions.

Context

Found while validating #2244. Unrelated to that PR's contents — it is a docs-only change that CI failed anyway, which is the point.