Do not retry 404 (MANIFEST_UNKNOWN) manifest fetches; classify as non-recoverable `not_found`

[bdehamer]

Summary

When the registry returns HTTP 404 (MANIFEST_UNKNOWN) for the image manifest — i.e. the image being validated does not exist in the registry — the provider retries the fetch the full --bundle-max-attempts times (default 3). A 404 is deterministic within the lifetime of a single admission request, so these retries never succeed; they just add two extra registry round-trips per failed validation. This is wasteful precisely during rollout bursts (when not-yet-pushed images are most common and registry load is already high).

The provider already has the information needed to distinguish a 404 from a retryable failure (timeout, cancellation, transient registry error) — it captures the HTTP status code — but it does not use it when deciding whether to retry. This issue proposes classifying 404 Not Found as a non-recoverable failure so the retry loop stops early.

Background: how classification and retry work today

Failures are represented by FetchError (pkg/fetcher/bundle.go), which carries a StatusCode field. The retry loop stops early only when the error is marked non-recoverable:

// retryBundle
var fe *FetchError
if errors.As(err, &fe) && !fe.Recoverable {
    fe.Attempts = attempts
    return nil, nil, fe
}

Recoverable is set in newFetchError, and it is derived only from the Kind — the HTTP status code is stored but not consulted:

func newFetchError(step Step, fallback FailureKind, err error) *FetchError {
    kind, code := classifyTransport(err)
    if kind == KindUnknown {
        kind = fallback
    }
    recoverable := kind != KindUnauthorized &&
        kind != KindForbidden &&
        kind != KindBundleInvalid
    return &FetchError{Step: step, Kind: kind, StatusCode: code, Recoverable: recoverable, Err: err}
}

classifyTransport maps recognized transport errors to a FailureKind, but only 401/403/429 get a dedicated kind. A 404 falls through to the final return KindUnknown, transportErr.StatusCode:

var transportErr *transport.Error
if errors.As(err, &transportErr) {
    switch transportErr.StatusCode {
    case http.StatusUnauthorized: return KindUnauthorized, transportErr.StatusCode
    case http.StatusForbidden:    return KindForbidden, transportErr.StatusCode
    case http.StatusTooManyRequests: return KindThrottled, transportErr.StatusCode
    }
    // ... diagnostic-code checks ...
    return KindUnknown, transportErr.StatusCode   // ← 404 lands here (code is preserved)
}

So a manifest-not-found produces:

FetchError{ Step: descriptor, Kind: descriptor_error /* fallback */, StatusCode: 404, Recoverable: true }

Because Kind is the descriptor_error fallback (not in the non-recoverable set) and the StatusCode: 404 is ignored, the loop retries all 3 attempts.

The problem

• A 404 is deterministic for the duration of one admission request, so retrying it in-request cannot succeed — it only issues extra round-trips.
• With --bundle-delay=0 (a common configuration), the retries fire nearly back-to-back, so even in the case where the image is about to be pushed, the sub-second in-request retry window is far too short to bridge the gap.
• 404s tend to cluster during rollouts (pods admitted referencing images that are missing, mistyped, deleted, or pinned to a digest that isn't in this registry), which is exactly when the registry is under the most load — so the wasted round-trips land at the worst time.

Observed example error (registry/name redacted):

status=404: GET https://registry.example.com/v2/app/manifests/sha256:<digest>:
MANIFEST_UNKNOWN: manifest sha256:<digest> is not found

Why not retrying is safe

Marking a 404 non-recoverable only stops the provider's in-request retry loop. It does not change the outcome of the request (the validation still fails — the image genuinely can't be verified because its manifest isn't present), and it does not affect recovery of a legitimately-racing push, because that recovery happens at the orchestrator admission layer — a fresh admission request some seconds later (a new pod), not inside the provider's single request. The provider's sub-second in-request retries were never bridging that gap anyway.

Net effect: fewer wasted registry round-trips, identical admission behavior.

Proposed implementation

Preferred approach — add a dedicated not_found classification (also improves observability, since 404 is currently conflated with genuinely-unclassified descriptor failures under descriptor_error):

1. Add a FailureKind in pkg/fetcher/bundle.go:

   // KindNotFound indicates the registry returned HTTP 404 (manifest/blob not found).
   KindNotFound FailureKind = "not_found"

2. Classify 404 in classifyTransport — add a case to the existing switch transportErr.StatusCode:

   case http.StatusNotFound:
       return KindNotFound, transportErr.StatusCode

   Handling it here (rather than in a single step constructor) means it applies uniformly to the descriptor, referrers, and blob steps, since all route through newFetchError.

3. Mark it non-recoverable in newFetchError:

   recoverable := kind != KindUnauthorized &&
       kind != KindForbidden &&
       kind != KindBundleInvalid &&
       kind != KindNotFound

Because classifyTransport now returns a concrete kind for 404 (not KindUnknown), it will no longer fall back to descriptor_error; the failure will surface as reason=not_found in logs and the aaop_attestations_retrieved_fail{reason=...} metric.

Testing

• pkg/fetcher/bundle_test.go — add a case asserting that a *transport.Error with StatusCode: 404 classifies as KindNotFound, is non-recoverable, and that retryBundle makes exactly one attempt (no retries) for a 404. Mirror the existing non-recoverable coverage for 401/403.
• pkg/provider/provider_test.go — assert the item error surfaces as error_fetching_bundle_not_found and the fail metric is labeled reason="not_found".
• scripts/integration_test.sh — parses aaop_attestations_retrieved_fail{reason="..."} by summing across reason series; confirm it still tolerates the new not_found label (it sums all series, so it should — just verify).

Acceptance criteria

• A 404 manifest fetch makes a single attempt (attempts == 1), not --bundle-max-attempts.
• The failure is reported with a distinct not_found reason in both logs and the fail metric.
• 401/403/429/timeout/cancellation behavior is unchanged.

Out of scope / notes

• Negative caching: if a future bundle cache ever caches negative (not-found) results, a 404 must not be cached as a durable negative — an image can legitimately appear shortly after a 404 (push races), so a cached negative could cause spurious failures after the image is available. This issue is only about the retry decision, not caching.
• --bundle-delay interaction: with a non-zero delay, in-request retries could in principle bridge a very short push race; even so, a 404 is better handled by failing fast and letting the orchestrator re-admit. If desired, the fast-fail could be made conditional, but the simpler unconditional behavior is recommended.

Relevant code

• pkg/fetcher/bundle.go — FailureKind constants, FetchError (has StatusCode), classifyTransport, newFetchError, retryBundle.
• pkg/fetcher/bundle_test.go, pkg/provider/provider_test.go — existing classification/recoverability and reason-label tests to extend.
• scripts/integration_test.sh — parses the reason-labeled fail metric.