From fe427c75e690eb9146372c593d184696b785fadd Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Tue, 20 Oct 2026 21:27:48 +0200 Subject: [PATCH] Cache Go modules that differ only in case apart Module zips are stored under the module's PURL, which lowercases golang names, and under the filename {last element}@{version}.zip. So github.com/Sirupsen/logrus and github.com/sirupsen/logrus shared one cache entry although upstream serves different zips, and the second one requested failed go's checksum check. A module path with capitals now gets a hash of its exact spelling in the filename. All-lowercase paths keep the plain name, so existing cache entries stay valid. A capitalised module cached before this change sits under its lowercase twin's plain filename. When a cached zip's upstream URL names a different module path, it is discarded and fetched again. Only the module path is compared, so a cache filled from another upstream.go host is kept. Fixes #466 --- internal/handler/go.go | 56 +++++++++- internal/handler/go_case_test.go | 135 +++++++++++++++++++++++ internal/server/retention_golang_test.go | 14 ++- 3 files changed, 200 insertions(+), 5 deletions(-) create mode 100644 internal/handler/go_case_test.go diff --git a/internal/handler/go.go b/internal/handler/go.go index c5284724..7a1eb862 100644 --- a/internal/handler/go.go +++ b/internal/handler/go.go @@ -1,11 +1,13 @@ package handler import ( + "crypto/sha256" + "encoding/hex" "errors" - "fmt" "net/http" "strings" + "github.com/git-pkgs/proxy/internal/database" "github.com/git-pkgs/proxy/internal/retention" "github.com/git-pkgs/registries/fetch" ) @@ -119,12 +121,13 @@ func (h *GoHandler) handleRequest(w http.ResponseWriter, r *http.Request) { func (h *GoHandler) handleDownload(w http.ResponseWriter, r *http.Request, module, version string) { // Decode module path (! followed by lowercase = uppercase) decodedModule := decodeGoModule(module) - filename := fmt.Sprintf("%s@%s.zip", lastComponent(decodedModule), version) + filename := goZipFilename(decodedModule, version) h.proxy.Logger.Info("go module download request", "module", decodedModule, "version", version) downloadURL := h.upstreamURL + r.URL.Path + h.discardOtherModulesZip(decodedModule, module, version, filename) result, err := h.proxy.GetOrFetchArtifactFromURL( r.Context(), "golang", decodedModule, version, filename, downloadURL, ) @@ -148,6 +151,55 @@ func (h *GoHandler) handleDownload(w http.ResponseWriter, r *http.Request, modul ServeArtifactRequest(w, r, result) } +// goZipFilename is the name a module zip is cached under. Module paths are +// case-sensitive, but the zip is stored under the module's PURL, which +// lowercases golang names, so github.com/Sirupsen/logrus and +// github.com/sirupsen/logrus share one package. A path with uppercase +// letters therefore gets a hash of its exact spelling in the filename. An +// all-lowercase path keeps the plain name, so existing cache entries stay +// valid. +func goZipFilename(module, version string) string { + base := lastComponent(module) + "@" + version + if module == strings.ToLower(module) { + return base + ".zip" + } + digest := sha256.Sum256([]byte(module)) + return base + "~" + hex.EncodeToString(digest[:goModuleHashBytes]) + ".zip" +} + +// goModuleHashBytes is how much of a module path's hash a cached zip's +// filename carries: enough to tell apart paths that differ only in case. +const goModuleHashBytes = 8 + +// discardOtherModulesZip drops a cached zip that another module stored +// under filename. Before module paths kept their case, a module with +// uppercase letters was cached under the plain filename of its lowercase +// twin, and whichever came first was served for both. The upstream URL a +// zip was fetched from names its module in escaped form, so a record whose +// URL names a different module is discarded and fetched again. Only the +// module path is compared, so pointing upstream.go elsewhere keeps the cache. +func (h *GoHandler) discardOtherModulesZip(module, escapedModule, version, filename string) { + _, versionPURL, err := packagePURLStrings("golang", module, version) + if err != nil { + return + } + record, err := h.proxy.DB.GetArtifact(versionPURL, filename) + if err != nil || record == nil || !record.StoragePath.Valid || record.Source == database.SourceLocal { + return + } + // Only a URL that names another Go module proves the zip is someone + // else's; a record of unknown origin is kept. + if !strings.Contains(record.UpstreamURL, "/@v/") || + strings.HasSuffix(record.UpstreamURL, "/"+escapedModule+"/@v/"+version+".zip") { + return + } + h.proxy.Logger.Warn("cached zip belongs to a module that differs only in case, discarding", + "module", module, "version", version, "cached_from", record.UpstreamURL) + if err := h.proxy.DB.DiscardArtifact(versionPURL, filename, record.StoragePath.String); err != nil { + h.proxy.Logger.Warn("failed to discard cached zip", "module", module, "version", version, "error", err) + } +} + // proxyCached forwards a request with metadata caching. func (h *GoHandler) proxyCached(w http.ResponseWriter, r *http.Request, cacheKey string) { h.proxy.ProxyCached(w, r, h.upstreamURL+r.URL.Path, "golang", cacheKey, "*/*") diff --git a/internal/handler/go_case_test.go b/internal/handler/go_case_test.go new file mode 100644 index 00000000..9451caa8 --- /dev/null +++ b/internal/handler/go_case_test.go @@ -0,0 +1,135 @@ +package handler + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + + "github.com/git-pkgs/registries/fetch" +) + +const ( + upperLogrusPath = "/github.com/!sirupsen/logrus/@v/v1.0.6.zip" + lowerLogrusPath = "/github.com/sirupsen/logrus/@v/v1.0.6.zip" +) + +func TestGoZipFilenameKeepsCase(t *testing.T) { + lower := goZipFilename("github.com/sirupsen/logrus", "v1.0.6") + if lower != "logrus@v1.0.6.zip" { + t.Errorf("lowercase module filename = %q, want the plain name", lower) + } + upper := goZipFilename("github.com/Sirupsen/logrus", "v1.0.6") + if upper == lower || !strings.HasPrefix(upper, "logrus@v1.0.6~") || !strings.HasSuffix(upper, ".zip") { + t.Errorf("uppercase module filename = %q, want logrus@v1.0.6~{hash}.zip", upper) + } + if other := goZipFilename("github.com/SIRUPSEN/logrus", "v1.0.6"); other == upper { + t.Errorf("differently cased modules share filename %q", other) + } +} + +// goCaseUpstream serves different bytes for each module path and counts +// requests per path. +func goCaseUpstream(t *testing.T) (*httptest.Server, map[string]*atomic.Int32) { + t.Helper() + counts := map[string]*atomic.Int32{upperLogrusPath: {}, lowerLogrusPath: {}} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + count, ok := counts[r.URL.Path] + if !ok { + http.NotFound(w, r) + return + } + count.Add(1) + _, _ = io.WriteString(w, "zip of "+r.URL.Path) + })) + t.Cleanup(srv.Close) + return srv, counts +} + +func newGoCaseHandler(t *testing.T, upstream *httptest.Server) (http.Handler, *Proxy) { + t.Helper() + proxy, _, store, _ := setupTestProxy(t) + store.seekable = true + proxy.HTTPClient = upstream.Client() + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + return NewGoHandlerWithUpstream(proxy, "http://localhost", upstream.URL).Routes(), proxy +} + +func getGo(h http.Handler, path string) *httptest.ResponseRecorder { + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, path, nil)) + return w +} + +func TestGoModulesDifferingInCaseAreCachedApart(t *testing.T) { + upstream, counts := goCaseUpstream(t) + h, _ := newGoCaseHandler(t, upstream) + + for range 2 { + for _, path := range []string{upperLogrusPath, lowerLogrusPath} { + if w := getGo(h, path); w.Code != http.StatusOK || w.Body.String() != "zip of "+path { + t.Fatalf("GET %s: status = %d, body = %q", path, w.Code, w.Body) + } + } + } + for path, count := range counts { + if n := count.Load(); n != 1 { + t.Errorf("%s fetched %d times, want once and then cached", path, n) + } + } +} + +// Before module paths kept their case, the uppercase module was cached under +// the lowercase module's plain filename. The lowercase module must not be +// served those bytes. +func TestGoZipCachedForAnotherModuleIsRefetched(t *testing.T) { + upstream, counts := goCaseUpstream(t) + h, proxy := newGoCaseHandler(t, upstream) + seedGoZip(t, proxy, "https://proxy.golang.org"+upperLogrusPath, "zip of "+upperLogrusPath) + + if w := getGo(h, lowerLogrusPath); w.Code != http.StatusOK || w.Body.String() != "zip of "+lowerLogrusPath { + t.Fatalf("status = %d, body = %q, want the lowercase module's zip", w.Code, w.Body) + } + if n := counts[lowerLogrusPath].Load(); n != 1 { + t.Errorf("lowercase module fetched %d times, want 1", n) + } +} + +// A zip cached from another upstream host for the same module stays valid: +// only the module path decides whether a cached zip belongs to the request. +func TestGoZipFromAnotherUpstreamHostIsKept(t *testing.T) { + upstream, counts := goCaseUpstream(t) + h, proxy := newGoCaseHandler(t, upstream) + seedGoZip(t, proxy, "https://old-proxy.example"+lowerLogrusPath, "zip of "+lowerLogrusPath) + + if w := getGo(h, lowerLogrusPath); w.Code != http.StatusOK || w.Body.String() != "zip of "+lowerLogrusPath { + t.Fatalf("status = %d, body = %q", w.Code, w.Body) + } + if n := counts[lowerLogrusPath].Load(); n != 0 { + t.Errorf("zip fetched %d times, want it served from the cache", n) + } +} + +// seedGoZip caches content as github.com/sirupsen/logrus v1.0.6 under the +// plain filename, recorded as fetched from upstreamURL. +func seedGoZip(t *testing.T, proxy *Proxy, upstreamURL, content string) { + t.Helper() + const storagePath = "golang/logrus/v1.0.6/seed/logrus@v1.0.6.zip" + store := proxy.Storage.(*mockStorage) + if _, _, err := store.Store(context.Background(), storagePath, strings.NewReader(content)); err != nil { + t.Fatalf("seeding storage: %v", err) + } + pkgPURL, versionPURL, err := packagePURLStrings("golang", "github.com/sirupsen/logrus", "v1.0.6") + if err != nil { + t.Fatalf("packagePURLStrings: %v", err) + } + artifact := testArtifact(content, versionPURL, "logrus@v1.0.6.zip", "application/zip") + if err := proxy.updateCacheDB("golang", "github.com/sirupsen/logrus", pkgPURL, upstreamURL, storagePath, artifact, ""); err != nil { + t.Fatalf("seeding cache record: %v", err) + } +} diff --git a/internal/server/retention_golang_test.go b/internal/server/retention_golang_test.go index 50c8ddef..f74eaf41 100644 --- a/internal/server/retention_golang_test.go +++ b/internal/server/retention_golang_test.go @@ -74,10 +74,18 @@ func golangGet(t *testing.T, url string) { } // golangArtifact returns the record of the zip named filename. +// golangFilenameMatch matches a zip's cached filename: the plain name, or, +// for a module path with capitals, the plain name with a hash of the path. +const golangFilenameMatch = `(filename = ? OR filename LIKE ?)` + +func golangFilenameArgs(filename string) []any { + return []any{filename, strings.TrimSuffix(filename, ".zip") + "~%.zip"} +} + func golangArtifact(t *testing.T, db *database.DB, filename string) database.Artifact { t.Helper() var art database.Artifact - if err := db.Get(&art, db.Rebind(`SELECT * FROM artifacts WHERE filename = ?`), filename); err != nil { + if err := db.Get(&art, db.Rebind(`SELECT * FROM artifacts WHERE `+golangFilenameMatch), golangFilenameArgs(filename)...); err != nil { t.Fatalf("reading artifact %s: %v", filename, err) } return art @@ -85,8 +93,8 @@ func golangArtifact(t *testing.T, db *database.DB, filename string) database.Art func golangBackdate(t *testing.T, db *database.DB, filename string, fetched, accessed time.Time) { t.Helper() - query := db.Rebind(`UPDATE artifacts SET fetched_at = ?, last_accessed_at = ? WHERE filename = ?`) - if _, err := db.Exec(query, fetched, accessed, filename); err != nil { + query := db.Rebind(`UPDATE artifacts SET fetched_at = ?, last_accessed_at = ? WHERE ` + golangFilenameMatch) + if _, err := db.Exec(query, append([]any{fetched, accessed}, golangFilenameArgs(filename)...)...); err != nil { t.Fatalf("backdating %s: %v", filename, err) } }