diff --git a/docs/configuration.md b/docs/configuration.md index ffd57fe2..afc0e6d3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -926,6 +926,8 @@ cache_metadata: true Or via environment variable: `PROXY_CACHE_METADATA=true`. +Composer archive downloads look up their upstream URL in the same cached metadata, so a cached archive is served without asking Packagist again and stays installable while it is down. Only a version or file the cached metadata does not list yet is looked up upstream. + The `proxy mirror` command always enables metadata caching regardless of this setting. ### Metadata TTL diff --git a/internal/handler/composer.go b/internal/handler/composer.go index 53a5a65a..121f228a 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -639,50 +639,6 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) return } - // We need to fetch the metadata to get the actual download URL since - // Packagist URLs include a hash. Packagist serves dev versions (e.g. - // "3.x-dev", "dev-master") from a separate "~dev" metadata file, while - // tagged releases live in the regular file. Try the file most likely to - // contain this version first, then fall back to the other so that both - // stable and dev versions resolve correctly. - metaURLs := h.metadataURLsForVersion(vendor, pkg, version) - - h.proxy.Logger.Debug("resolving download URL", - "package", packageName, "version", version, - "metadata_urls", metaURLs) - - var dist composerDist - for _, metaURL := range metaURLs { - found, err := h.findDownloadURLFromMetadata(r.Context(), metaURL, packageName, version) - if err != nil { - if writeUpstreamHeld(w, err) { - return - } - h.proxy.Logger.Error("failed to fetch metadata", "error", err, "url", metaURL) - http.Error(w, "failed to fetch metadata", http.StatusBadGateway) - return - } - if found.url != "" { - dist = found - break - } - } - - if dist.url == "" { - h.proxy.Logger.Debug("version not found in any metadata source", - "package", packageName, "version", version, - "tried_urls", metaURLs) - http.Error(w, "version not found", http.StatusNotFound) - return - } - - // Lock files name archive URLs directly, so a version filtered out of the - // metadata still has to be withheld here. - if h.shouldFilterVersion(canonicalPackagePURL("composer", packageName), packageName, version, dist.published) { - http.Error(w, "version not found", http.StatusNotFound) - return - } - // The archive is cached under the file name from the path, so fetch the // one that name stands for. Compare it as sent, the way rewriteDist handed // it out, query included. @@ -690,13 +646,18 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) if r.URL.RawQuery != "" { requested += "?" + r.URL.RawQuery } - listedURL := dist.url - downloadURL, ok := upstreamDistURL(listedURL, dist.distType, requested, isDevVersion(version)) + files := h.metadataFilesForVersion(vendor, pkg, version) + dist, downloadURL, ok := h.cachedDist(r.Context(), files, packageName, version, requested) if !ok { - h.proxy.Logger.Info("composer file not in upstream metadata", - "package", packageName, "version", version, - "filename", requested, "listed_url", listedURL) - http.Error(w, "file not found", http.StatusNotFound) + if dist, downloadURL, ok = h.upstreamDist(w, r, files, packageName, version, requested); !ok { + return + } + } + + // Lock files name archive URLs directly, so a version filtered out of the + // metadata still has to be withheld here. + if h.shouldFilterVersion(canonicalPackagePURL("composer", packageName), packageName, version, dist.published) { + http.Error(w, "version not found", http.StatusNotFound) return } @@ -718,7 +679,7 @@ func (h *ComposerHandler) handleDownload(w http.ResponseWriter, r *http.Request) h.discardOtherCommit(packageName, version, cacheFilename, commit) } result, err := h.proxy.GetOrFetchArtifactFromURLWithIntegrity(r.Context(), "composer", packageName, version, cacheFilename, downloadURL, - composerIntegrity(dist.shasum, version, downloadURL == listedURL)) + composerIntegrity(dist.shasum, version, downloadURL == dist.url)) if err != nil { h.proxy.serveArtifactError(w, err, "failed to fetch package") return @@ -767,19 +728,98 @@ func isDevVersion(version string) bool { return strings.HasPrefix(version, "dev-") || strings.HasSuffix(version, "-dev") } -// metadataURLsForVersion returns the upstream metadata URLs to consult for a +// composerMetadataFile is one of the metadata documents a version's dist can +// be listed in: its upstream URL and the key the proxy caches it under. +type composerMetadataFile struct { + url, cacheKey string +} + +// metadataFilesForVersion returns the metadata documents to consult for a // given version, in priority order. Dev versions are served from the "~dev" // file, tagged releases from the regular file; the other file is included as a -// fallback so an unexpected classification still resolves. -func (h *ComposerHandler) metadataURLsForVersion(vendor, pkg, version string) []string { - repoURL, _ := h.sourceFor(vendor + "/" + pkg) - stable := fmt.Sprintf("%s/p2/%s/%s.json", repoURL, vendor, pkg) - dev := fmt.Sprintf("%s/p2/%s/%s~dev.json", repoURL, vendor, pkg) +// fallback so an unexpected classification still resolves. The cache keys are +// the ones handlePackageMetadata stores the documents under. +func (h *ComposerHandler) metadataFilesForVersion(vendor, pkg, version string) []composerMetadataFile { + file := func(name string) composerMetadataFile { + repoURL, cacheKey := h.sourceFor(name) + return composerMetadataFile{url: fmt.Sprintf("%s/p2/%s.json", repoURL, name), cacheKey: cacheKey} + } + stable := file(vendor + "/" + pkg) + dev := file(vendor + "/" + pkg + composerDevFile) if isDevVersion(version) { - return []string{dev, stable} + return []composerMetadataFile{dev, stable} + } + return []composerMetadataFile{stable, dev} +} + +// cachedDist looks up the dist of version in the metadata documents the proxy +// caches for Composer, returning it with the URL to download the requested +// file from. While the documents are fresh this costs no upstream request, +// and when upstream fails their cached copies still resolve the archive. ok is +// false when neither document lists a dist the requested file stands for, as +// for a version published after the metadata was cached, so the caller asks +// upstream instead. Without cache_metadata there is nothing to look in. +func (h *ComposerHandler) cachedDist(ctx context.Context, files []composerMetadataFile, packageName, version, requested string) (composerDist, string, bool) { + if !h.proxy.CacheMetadata || h.proxy.DB == nil { + return composerDist{}, "", false + } + for _, f := range files { + doc := h.proxy.fetchMetadataDocument(ctx, "composer", f.cacheKey, f.url, contentTypeJSON) + if doc.err != nil { + continue + } + dist, err := composerDistURL(doc.body, packageName, version) + if err != nil || dist.url == "" { + continue + } + if downloadURL, ok := upstreamDistURL(dist.url, dist.distType, requested, isDevVersion(version)); ok { + return dist, downloadURL, true + } + } + return composerDist{}, "", false +} + +// upstreamDist looks up the dist of version in the metadata upstream serves +// now, returning it with the URL to download the requested file from. When it +// cannot, it writes the error response and returns false. +func (h *ComposerHandler) upstreamDist(w http.ResponseWriter, r *http.Request, files []composerMetadataFile, packageName, version, requested string) (composerDist, string, bool) { + h.proxy.Logger.Debug("resolving download URL", + "package", packageName, "version", version) + + var dist composerDist + for _, f := range files { + found, err := h.findDownloadURLFromMetadata(r.Context(), f.url, packageName, version) + if err != nil { + if writeUpstreamHeld(w, err) { + return composerDist{}, "", false + } + h.proxy.Logger.Error("failed to fetch metadata", "error", err, "url", f.url) + http.Error(w, "failed to fetch metadata", http.StatusBadGateway) + return composerDist{}, "", false + } + if found.url != "" { + dist = found + break + } + } + + if dist.url == "" { + h.proxy.Logger.Debug("version not found in any metadata source", + "package", packageName, "version", version) + http.Error(w, "version not found", http.StatusNotFound) + return composerDist{}, "", false + } + + downloadURL, ok := upstreamDistURL(dist.url, dist.distType, requested, isDevVersion(version)) + if !ok { + h.proxy.Logger.Info("composer file not in upstream metadata", + "package", packageName, "version", version, + "filename", requested, "listed_url", dist.url) + http.Error(w, "file not found", http.StatusNotFound) + return composerDist{}, "", false } - return []string{stable, dev} + return dist, downloadURL, true } // composerDist is what a version's metadata says about its archive: the dist diff --git a/internal/handler/composer_download_lookup_test.go b/internal/handler/composer_download_lookup_test.go new file mode 100644 index 00000000..5ac4c75c --- /dev/null +++ b/internal/handler/composer_download_lookup_test.go @@ -0,0 +1,128 @@ +package handler + +import ( + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "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. +type composerLookupUpstream struct { + h http.Handler + versions atomic.Value // []string + down atomic.Bool + metadata atomic.Int32 + archives atomic.Int32 +} + +func setupComposerLookup(t *testing.T, cacheMetadata bool, versions ...string) *composerLookupUpstream { + t.Helper() + u := &composerLookupUpstream{} + u.versions.Store(versions) + 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) + return + } + switch { + case r.URL.Path == "/p2/v/p.json": + u.metadata.Add(1) + var entries []string + for _, v := range u.versions.Load().([]string) { + entries = append(entries, fmt.Sprintf(`{"version":%q,"dist":{"url":%q,"type":"zip"}}`, v, srv.URL+"/archives/"+v+".zip")) + } + _, _ = fmt.Fprintf(w, `{"packages":{"v/p":[%s]}}`, strings.Join(entries, ",")) + case r.URL.Path == "/p2/v/p~dev.json": + u.metadata.Add(1) + http.NotFound(w, r) + case strings.HasPrefix(r.URL.Path, "/archives/"): + u.archives.Add(1) + _, _ = io.WriteString(w, "archive "+r.URL.Path) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + + proxy, _, _, _ := setupTestProxy(t) + proxy.CacheMetadata = cacheMetadata + proxy.MetadataTTL = time.Hour + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(srv.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + u.h = NewComposerHandlerWithUpstreams(proxy, "http://proxy", srv.URL, srv.URL).Routes() + return u +} + +func (u *composerLookupUpstream) get(t *testing.T, target string) *httptest.ResponseRecorder { + t.Helper() + w := httptest.NewRecorder() + u.h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) + return w +} + +func (u *composerLookupUpstream) download(t *testing.T, version string) { + t.Helper() + w := u.get(t, "/files/v/p/"+version+"/"+version+".zip") + if want := "archive /archives/" + version + ".zip"; w.Code != http.StatusOK || w.Body.String() != want { + t.Fatalf("download %s: status %d, body %q, want 200 %q", version, w.Code, w.Body.String(), want) + } +} + +// TestComposerDownloadResolvesFromCachedMetadata checks that downloads find +// their archive in the metadata Composer was just served, instead of fetching +// it from upstream again for every archive. +func TestComposerDownloadResolvesFromCachedMetadata(t *testing.T) { + u := setupComposerLookup(t, true, "1.0.0") + if w := u.get(t, "/p2/v/p.json"); w.Code != http.StatusOK { + t.Fatalf("metadata: status %d", w.Code) + } + u.download(t, "1.0.0") + u.download(t, "1.0.0") + if n := u.metadata.Load(); n != 1 { + t.Errorf("upstream metadata requests = %d, want 1 (the metadata request itself)", n) + } + if n := u.archives.Load(); n != 1 { + t.Errorf("upstream archive requests = %d, want 1", n) + } +} + +// TestComposerCachedArchiveServedWhileUpstreamDown checks that a cached +// archive stays installable when upstream fails, rather than failing on the +// metadata lookup that only names where to fetch it from. +func TestComposerCachedArchiveServedWhileUpstreamDown(t *testing.T) { + u := setupComposerLookup(t, true, "1.0.0") + u.download(t, "1.0.0") + u.down.Store(true) + u.download(t, "1.0.0") +} + +// TestComposerDownloadOfVersionNewerThanCachedMetadata checks that a version +// published after the metadata was cached, which a lock file can name before +// the cached copy expires, is still looked up upstream. +func TestComposerDownloadOfVersionNewerThanCachedMetadata(t *testing.T) { + u := setupComposerLookup(t, true, "1.0.0") + u.download(t, "1.0.0") + u.versions.Store([]string{"1.0.0", "1.1.0"}) + u.download(t, "1.1.0") +} + +// TestComposerDownloadWithoutMetadataCache checks that without cache_metadata +// a download asks upstream for the metadata once, as it always has. +func TestComposerDownloadWithoutMetadataCache(t *testing.T) { + u := setupComposerLookup(t, false, "1.0.0") + u.download(t, "1.0.0") + if n := u.metadata.Load(); n != 1 { + t.Errorf("upstream metadata requests = %d, want 1", n) + } +} diff --git a/internal/handler/composer_test.go b/internal/handler/composer_test.go index e6a99626..d2955eb9 100644 --- a/internal/handler/composer_test.go +++ b/internal/handler/composer_test.go @@ -505,16 +505,16 @@ func TestComposerDownloadDevVersionUsesDevMetadata(t *testing.T) { // FIXED behavior: the handler consults the ~dev file (it is first in the // candidate list for dev versions) and resolves the dist URL. - urls := h.metadataURLsForVersion(vendor, name, version) - if len(urls) == 0 || !strings.HasSuffix(urls[0], "/p2/phpmd/phpmd~dev.json") { - t.Fatalf("dev version should consult the ~dev metadata file first, got %v", urls) + files := h.metadataFilesForVersion(vendor, name, version) + if len(files) == 0 || !strings.HasSuffix(files[0].url, "/p2/phpmd/phpmd~dev.json") || files[0].cacheKey != "phpmd/phpmd~dev" { + t.Fatalf("dev version should consult the ~dev metadata file first, got %v", files) } var resolved string - for _, u := range urls { - found, err := h.findDownloadURLFromMetadata(ctx, u, pkg, version) + for _, f := range files { + found, err := h.findDownloadURLFromMetadata(ctx, f.url, pkg, version) if err != nil { - t.Fatalf("unexpected error fetching metadata %q: %v", u, err) + t.Fatalf("unexpected error fetching metadata %q: %v", f.url, err) } resolved = found.url if resolved != "" {