From 9dafb8cb361916cf164ae13f1717534595eea102 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 17 Oct 2026 13:58:36 +0200 Subject: [PATCH] Resolve Composer downloads from the cached metadata Every Composer archive download fetched the package's p2 metadata straight from upstream to find the archive's URL, before the archive cache was even checked. A cached archive cost one or two uncached Packagist requests per package and install, and failed with a 502 while Packagist was down. With cache_metadata on, downloads now look the dist up in the metadata the proxy already caches for Composer, under the keys handlePackageMetadata uses. Within the TTL that needs no upstream request, and when upstream fails the stale copy still resolves the archive. A version or file the cached documents do not list, such as one published since they were cached, is looked up upstream as before. --- docs/configuration.md | 2 + internal/handler/composer.go | 158 +++++++++++------- .../handler/composer_download_lookup_test.go | 128 ++++++++++++++ internal/handler/composer_test.go | 12 +- 4 files changed, 235 insertions(+), 65 deletions(-) create mode 100644 internal/handler/composer_download_lookup_test.go 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 != "" {