[medlabib]

Fixes #499.

With cache_metadata on, npm metadata served through the rewrite path (cachedRewrite → serveRewrittenMetadata) came back as a plain 200 even when the body was the cached copy after an upstream failure: no Warning: 110 - "Response is Stale", unlike ProxyCached on the same fallback. Separately, a 404 for an unpublished package and a 401/403 for credentials the registry rejected were swallowed by the cache fallback, so neither answer reached the client.

What changed:

• rewrittenMetadata carries stale, set from the fetch result in handlePackageMetadata, and serveRewrittenMetadata sets Warning: 110 - "Response is Stale" when it is set. A 304 keeps the headers it has today.
• 401 and 403 are carried as an upstreamStatusError, so the answer is distinguishable from a transport failure.
• The npm handler relays a definitive upstream answer (404, 401, 403) instead of the cached copy. A 404 keeps the existing per-package/version JSON error; default-registry 401/403 are relayed as that status with a JSON error body. Routed packages keep the existing 502: a route's credentials are the proxy's concern, and relaying its 401 would send npm off to authenticate against the proxy. TestNPMRoutedMetadataDoesNotFallBack still passes.
• The no-fallback rule is scoped to the npm ecosystem. fetchMetadataFromUpstream keeps its documented stale fallback for Composer, PyPI, Alpine, Debian and the other callers, including on 404/401/403; only npm returns the answer instead of the cached copy. npm still falls back on transport failures and retryable statuses (429, 5xx).

Tests, in the new internal/handler/npm_metadata_stale_test.go:

• the stale fallback sets the Warning header with the rewrite cache on and off, and returns the cached body (this fails on main: no header, and 404/401/403 are served as a stale 200);
• a fresh response carries no Warning;
• 404, 401 and 403 reach the npm client as those statuses, with a JSON error body and not the cached packument;
• a 503 still serves the cached copy with the Warning, guarding the rate-limit/hold fallback;
• regression test through the generic (non-npm) metadata handler: a cached composer document is still served with the Warning when upstream answers 404/401/403. This test fails if the pass-through is not scoped to npm.

go test ./... -count=1        # all packages pass
golangci-lint run ./...       # 0 issues (golangci-lint v2.13.1)

Not included: a maximum stale age for the fallback, which the issue mentions as optional.

AI disclosure: prepared with AI assistance (Hermes agent); the change was verified with the test suite and lint run above.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

[simonchrz]

Thanks for picking up #499. The stale Warning header for rewritten npm metadata looks good.

The no-fallback change affects every ecosystem, though, not just npm. fetchMetadataFromUpstream also serves Composer, PyPI, Alpine, Debian and generic metadata. On main, cached metadata is served on an upstream 404/401/403. With this PR those requests fail. That contradicts the docs (generic upstreams are served stale when the upstream "refuses", and GitHub rate limits are 403s) and the 404 fallback described in #486.

Could you split it? I'd approve the Warning header part as is. The 404/401/403 behaviour either goes to an issue, or becomes opt-in for the npm handler only.

Nit: writeNPMetadataError is missing an "M".

[andrew]

I independently confirmed the shared-fallback regression described in #503 (review). Through the generic HTTP handler with an expired cached document, upstream 404/401/403 produce 404/502/502 on this head. The same test on main returns the cached body with status 200 and Warning 110 for all three. Please keep the stale-warning fix and scope the new fallback policy to npm, or split that policy into a separate change, preserving other ecosystems' documented fallback behavior. Add a regression test through a non-npm handler.

[simonchrz]

Thanks, scoping the fallback to npm addresses my earlier review. Two things left, both on the version path handleFilteredNPMVersionMetadata takes when cooldown or a version denylist is on:

1. The stale flag is dropped there. With a denylist entry for another version, /left-pad/1.0.0 returns the cached metadata as 200 with no Warning after an upstream 503, while /left-pad gets the warning. It reads the packument through FetchOrCacheMetadata, which discards stale. Switching it to fetchMetadataDocument and setting rewritten.stale would fix it.
2. A 401 from the default registry is relayed on /left-pad but becomes 502 on /left-pad/1.0.0. I'd also prefer 502 on both. The 401 means the upstream rejected the proxy's credentials, and relaying it makes npm ask the user to log in to the proxy, which can't fix it.

A regression test through the npm route with a denylist entry would cover both.

[andrew]

The npm scoping and the non-npm regression test cover my earlier review, thanks. @simonchrz's two points from this morning still stand. Two more things on top of those:

401/403 from the default registry: answer 502 on every npm path. Keep relaying 404. A 401 or 403 there means the registry rejected the proxy's own credentials, and the client can't fix that. If the proxy relays a 401, npm tells the user to log in to the proxy. The no-fallback rule sits in the shared fetchMetadataFromUpstream, so the filtered version path and hosted serveMetadataWithLocal (npm_hosted.go) already return 502 on this branch. Only handlePackageMetadata relays. Please drop the upstreamStatusError branch from writeNPMMetadataError so all three paths return 502, put the upstream status in the error body (for example upstream rejected the proxy's credentials (401)), and change TestNPMMetadataPassesDefinitiveUpstreamAnswersThrough to expect 502 for 401/403.

Use the stale helper that's now on main. #541 merged today and added staleWarning and markStale(w, stale) in handler.go, which callers run just before serveRewrittenMetadata. Please use those instead of the stale field on rewrittenMetadata and the header literal in rewrite_cache.go: call markStale(w, upstream.stale) in handlePackageMetadata, and compare against staleWarning in the tests. For the filtered version path, switch to fetchMetadataDocument and call markStale there too, with a test through the npm route that has a denylist entry.