diff --git a/internal/handler/composer.go b/internal/handler/composer.go index ccb42bbb..aa790808 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -905,9 +905,9 @@ type composerDist struct { } // findDownloadURLFromMetadata fetches a metadata document and returns the dist -// of the given version, with an empty URL if the version is not present. An -// error is returned only on transport failure; a missing document (non-200) -// or a missing version both yield an empty URL so the caller can fall back. +// of the given version, with an empty URL if the version is not present. A +// missing document (404 or 410) or a missing version both yield an empty URL +// so the caller can fall back; any other failure is an error. func (h *ComposerHandler) findDownloadURLFromMetadata(ctx context.Context, metaURL, packageName, version string) (composerDist, error) { h.proxy.Logger.Debug("fetching upstream metadata for download lookup", "url", metaURL, "package", packageName, "version", version) @@ -926,8 +926,16 @@ func (h *ComposerHandler) findDownloadURLFromMetadata(ctx context.Context, metaU h.proxy.Logger.Debug("upstream metadata response", "url", metaURL, "status", resp.StatusCode) - if resp.StatusCode != http.StatusOK { + switch { + case resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusGone: return composerDist{}, nil + case resp.StatusCode == http.StatusTooManyRequests && h.proxy.RateLimits != nil: + if err := h.proxy.RateLimits.Held(metaURL); err != nil { + return composerDist{}, err + } + } + if resp.StatusCode != http.StatusOK { + return composerDist{}, fmt.Errorf("upstream metadata returned %d", resp.StatusCode) } body, err := h.proxy.ReadMetadata(resp.Body) diff --git a/internal/handler/composer_download_lookup_test.go b/internal/handler/composer_download_lookup_test.go index 56350004..53a0cfe0 100644 --- a/internal/handler/composer_download_lookup_test.go +++ b/internal/handler/composer_download_lookup_test.go @@ -11,23 +11,25 @@ import ( "time" "github.com/git-pkgs/cooldown" + registryclient "github.com/git-pkgs/registries/client" "github.com/git-pkgs/registries/fetch" ) // composerLookupUpstream serves v/p's metadata listing versions and the // archives they point at, counts metadata and archive requests, and fails -// every request while down is set. A version listed in published carries that -// publish time. +// every request while down is set, with downStatus or 503. A version listed in +// published carries that publish time. type composerLookupUpstream struct { - h http.Handler - handler *ComposerHandler - proxy *Proxy - versions atomic.Value // []string - published map[string]time.Time - down atomic.Bool - metadata atomic.Int32 - archives atomic.Int32 - routeURL string + h http.Handler + handler *ComposerHandler + proxy *Proxy + versions atomic.Value // []string + published map[string]time.Time + down atomic.Bool + downStatus atomic.Int32 + metadata atomic.Int32 + archives atomic.Int32 + routeURL string } func setupComposerLookup(t *testing.T, cacheMetadata bool, versions ...string) *composerLookupUpstream { @@ -37,7 +39,12 @@ func setupComposerLookup(t *testing.T, cacheMetadata bool, versions ...string) * var srv *httptest.Server srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if u.down.Load() { - http.Error(w, "down", http.StatusServiceUnavailable) + status := int(u.downStatus.Load()) + if status == 0 { + status = http.StatusServiceUnavailable + } + w.Header().Set("Retry-After", "30") + http.Error(w, "down", status) return } switch { @@ -146,6 +153,42 @@ func TestComposerDownloadWithoutMetadataCache(t *testing.T) { } } +// TestComposerDownloadLookupUpstreamError checks that a download whose +// metadata lookup fails upstream answers 502, or 503 while upstream holds +// requests, and not 404: Composer treats 404 as the version being gone. +func TestComposerDownloadLookupUpstreamError(t *testing.T) { + for _, status := range []int{http.StatusInternalServerError, http.StatusServiceUnavailable} { + t.Run(http.StatusText(status), func(t *testing.T) { + u := setupComposerLookup(t, false, "1.0.0") + u.downStatus.Store(int32(status)) + u.down.Store(true) + if w := u.get(t, "/files/v/p/1.0.0/1.0.0.zip"); w.Code != http.StatusBadGateway { + t.Errorf("status = %d, want 502", w.Code) + } + }) + } + + t.Run("rate limited", func(t *testing.T) { + u := setupComposerLookup(t, false, "1.0.0") + gate := registryclient.NewRateLimitGate(http.DefaultTransport) + u.proxy.HTTPClient = &http.Client{Transport: gate} + u.proxy.RateLimits = gate + u.downStatus.Store(http.StatusTooManyRequests) + u.down.Store(true) + w := u.get(t, "/files/v/p/1.0.0/1.0.0.zip") + if w.Code != http.StatusServiceUnavailable || w.Header().Get("Retry-After") == "" { + t.Errorf("status = %d, Retry-After %q; want 503 with Retry-After", w.Code, w.Header().Get("Retry-After")) + } + }) + + t.Run("not listed", func(t *testing.T) { + u := setupComposerLookup(t, false, "1.0.0") + if w := u.get(t, "/files/v/p/2.0.0/2.0.0.zip"); w.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404", w.Code) + } + }) +} + // storedPublishTime returns the publish time recorded for v/p at version. func (u *composerLookupUpstream) storedPublishTime(t *testing.T, version string) (time.Time, bool) { t.Helper()