Resolve Composer downloads from the cached metadata

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

internal/handler/composer.go

@@ -639,64 +639,25 @@ 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.
 	requested := path.Base(r.URL.EscapedPath())
 	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

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)
+	}
+}

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 != "" {