[ThomasSteinbach]

When upstream answers a metadata fetch with a definite 4xx (other than 404), the proxy still returns 502, which is right per #503's discussion, but it logs every one at error level. With install scripts that probe an object store for keys that may not exist, S3 answers 403 and each probe shows up as an error, so real outages get buried.

This logs those refusals as warnings and keeps 408/429 and 5xx at error. Status codes and the stale fallback don't change.

(Disclaimer: Got support from my buddy Opus 5.5)

Summary by CodeRabbit

• Bug Fixes
  • Upstream metadata failures are now classified more consistently in service logs. Refusal responses are logged as warnings, while timeouts, rate limits, and server errors are logged as errors.
  • The response to these failures remains 502 Bad Gateway; there is no change to the user-facing response.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Upstream metadata status errors now carry the status code and classify most 4xx responses as refusals. The fetch-error handler logs refusals as warnings and other failures as errors. It continues to return 502 Bad Gateway.

Changes: Metadata fetch error logging

Layer / File(s)	Summary
Status classification and fetch-error logging internal/handler/handler.go, internal/handler/metadata_fetch_error_test.go	Non-200, 304, and 404 statuses use a typed error. It classifies 4xx statuses as refusals except 408 and 429. The handler logs refusals as warnings and other failures as errors, while keeping the 502 response. Tests check the response, status in the log, and log level.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: simonchrz, andrew

Merge Risk: Minimal · up to 1446b

This change logs routine upstream refusals as warnings instead of errors. The client-facing 502 response is unchanged, so user impact is minimal and the change looks safe to merge.

Security architecture risk: Minimal · up to 1446b

Upstream refusals remain failures with the same client response. The change adjusts their log severity without introducing a material change to access controls, authority, persistence, or failure containment.

Pre-merge checks: 4

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title says the change logs upstream refusals of metadata fetches as warnings. The raw summary says writeMetadataFetchError logs refusals (4xx except 408 and 429) as warnings and other fetch failur…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title says the change logs upstream refusals of metadata fetches as warnings. The raw summary says writeMetadataFetchError logs refusals (4xx except 408 and 429) as warnings and other fetch failures as errors, and the test checks WARN for 403, 401, and 410. The title matches the main change and is specific enough for a teammate scanning history.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 08058ae1-2059-4c4a-b082-e185a5baef44

Commits

Reviewing files that changed from the base of the PR and between 57428c5 and 1446b0a.

Files selected for processing: 2

• internal/handler/handler.go
• internal/handler/metadata_fetch_error_test.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[simonchrz]

Thanks, this fixes a real nuisance, and putting it in writeMetadataFetchError covers the generic handler's ProxyCached path, which is where an object store's 403s land.

Two things to consider:

• Upstreams with credentials. When upstream.auth has credentials for the upstream, a 401 or 403 more likely means the proxy's own token expired or lacks a scope than a client probing for a key, and that is worth an error. Would it make sense to keep 401/403 at error level for upstreams with configured auth?
• Other handlers. Several handlers log a failed metadata fetch with their own Logger.Error and don't go through writeMetadataFetchError. The PyPI JSON API, for example, logs "upstream request failed". They still log a 403 as an error. That's fine for the case you describe, but the title reads broader, so maybe say in the description that it covers the paths through writeMetadataFetchError.

A test through a handler, for example the generic one with an upstream answering 403, would pin the case from the description. The current test calls the internal functions directly.