From 39d178f62938a5dba2bf4b5a05c9de14d7bf60e1 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Fri, 30 Oct 2026 09:33:56 +0100 Subject: [PATCH 1/8] Expire cooldown rewrites when a withheld version becomes available A cached rewrite made with cooldown on stays right until a version it held back leaves cooldown, so it now expires then instead of after a fixed time. Handlers ask cooldown through Proxy.cooldown, which reports each withheld version to the rewrites running at that moment. A rewrite that held nothing back is kept for the shortest cooldown, at most a day. --- docs/configuration.md | 4 +- internal/handler/cargo.go | 2 +- internal/handler/composer.go | 4 +- internal/handler/conda.go | 4 +- internal/handler/cooldown_expiry.go | 164 +++++++++++++++++ internal/handler/cooldown_expiry_test.go | 178 +++++++++++++++++++ internal/handler/cooldown_times.go | 2 +- internal/handler/go_cooldown.go | 2 +- internal/handler/handler.go | 4 + internal/handler/helm.go | 2 +- internal/handler/maven_cooldown.go | 6 +- internal/handler/metadata_access_test.go | 2 +- internal/handler/metadata_gzip_test.go | 2 +- internal/handler/metadata_validators_test.go | 2 +- internal/handler/npm.go | 6 +- internal/handler/npm_rewrite_test.go | 4 +- internal/handler/nuget_cooldown.go | 2 +- internal/handler/pub.go | 2 +- internal/handler/pypi.go | 8 +- internal/handler/rewrite_cache.go | 62 ++----- internal/handler/rewrite_cache_test.go | 43 +++-- 21 files changed, 413 insertions(+), 92 deletions(-) create mode 100644 internal/handler/cooldown_expiry.go create mode 100644 internal/handler/cooldown_expiry_test.go diff --git a/docs/configuration.md b/docs/configuration.md index 941bc0f0..7f622071 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1141,7 +1141,7 @@ Maven caveats: RubyGems, Hex and PyPI keep their publish times outside the metadata clients read: the RubyGems versions API, the Hex HTTP API (`upstream.hex_api`) and the PyPI JSON API. The proxy fetches them alongside each compact index, package or simple page it filters, and for PyPI downloads. Cooldown fails closed: when it applies to a package and those times can't be fetched or parsed, or the metadata can't be rewritten, the request fails with 502, or with 503 and `Retry-After` while the upstream is rate limiting the proxy, instead of serving the metadata unfiltered. The same goes for npm, Composer and pub.dev metadata that can't be rewritten, and for npm tarball downloads whose publish time can't be read. A PyPI file or npm tarball that is already cached is still served when its publish time can't be read; only a download that would have to be fetched fails. A PyPI release's time is the upload time of its newest file. The proxy stores it when a download looks it up, and serves files of that release that are already cached against the stored time without reading the JSON API again. A file that isn't cached yet is always looked up, since it may have been added to the release since, and the newer time is stored. Packages exempted with a duration of `0` are served as before and don't need the times. A PyPI package the JSON API returns 404 for has no times to go by and is served unfiltered. -For RubyGems and Hex, both documents go through the metadata cache. With [`cache_metadata`](#metadata-caching) they are served from it within `metadata_ttl` without asking the upstream, revalidated with their ETags after it, and filtered from the last cached copies, marked stale, while the upstream is down; without it they are still fetched once for concurrent requests. Each document has its own TTL, so the compact index or Hex package can be newer than the cached times and list a version they don't have. The times are then fetched again before filtering, and if that fails, such versions are left out until the times can be read. The filtered index or package carries an ETag of its own, so Bundler and Mix revalidate it with `304 Not Modified`. Publish times are compared with the current time on each request, but the filtered result is kept for up to an hour (see [rewritten metadata cache](#rewritten-metadata-cache)), so a version leaving cooldown can show up that much later; a newly published version shows up within `metadata_ttl`, as with other cached metadata. Upstream errors are answered as for other cached metadata (404, or 502) rather than relayed. +For RubyGems and Hex, both documents go through the metadata cache. With [`cache_metadata`](#metadata-caching) they are served from it within `metadata_ttl` without asking the upstream, revalidated with their ETags after it, and filtered from the last cached copies, marked stale, while the upstream is down; without it they are still fetched once for concurrent requests. Each document has its own TTL, so the compact index or Hex package can be newer than the cached times and list a version they don't have. The times are then fetched again before filtering, and if that fails, such versions are left out until the times can be read. The filtered index or package carries an ETag of its own, so Bundler and Mix revalidate it with `304 Not Modified`. Publish times are compared with the current time on each request, and the filtered result is kept until the first version it held back leaves cooldown (see [rewritten metadata cache](#rewritten-metadata-cache)); a newly published version shows up within `metadata_ttl`, as with other cached metadata. Upstream errors are answered as for other cached metadata (404, or 502) rather than relayed. With cooldown on, the proxy asks npm for full packuments, since the abbreviated ones it uses otherwise carry no publish times. Abbreviated packuments cached before cooldown was turned on are fetched again in full rather than served, including as a stale copy when the upstream is down. When `latest` is in cooldown, the packument tags the newest version out of cooldown instead, and `npm dist-tag ls` shows the same tags. @@ -1420,7 +1420,7 @@ Or via environment variable: `PROXY_ARTIFACT_MAX_SIZE=2GB`. ### Rewritten metadata cache -The npm, Composer, PyPI and Helm handlers rewrite every metadata document they serve, so that download URLs point at the proxy. That means decoding the whole document and encoding it again, and Composer's minified documents are also expanded, which together cost milliseconds per typical package and far more for very large ones; a large Helm `index.yaml` takes over a hundred milliseconds. The proxy keeps rewritten documents in memory, so each distinct upstream document is rewritten once and requests arriving while it is being rewritten wait for that rewrite. New bytes from upstream are rewritten again. A PyPI simple page is cached per representation (HTML or JSON) and per set of versions filtered out of it. With version cooldown enabled, a cached rewrite expires, because cooldown filtering depends on the current time: it is kept for a 24th of the shortest cooldown configured, at least a minute and at most an hour, so a version shows up at most that long after its cooldown ends. With a cooldown of a day or more that is an hour; with `"2h"` somewhere, five minutes. Cached documents carry an `ETag` of their own, and are dated like any other, so clients that revalidate with `If-None-Match` or `If-Modified-Since`, such as Composer, get `304 Not Modified`. The RubyGems compact index and Hex packages filtered by cooldown are kept the same way; they are never served gzip-compressed. +The npm, Composer, PyPI and Helm handlers rewrite every metadata document they serve, so that download URLs point at the proxy. That means decoding the whole document and encoding it again, and Composer's minified documents are also expanded, which together cost milliseconds per typical package and far more for very large ones; a large Helm `index.yaml` takes over a hundred milliseconds. The proxy keeps rewritten documents in memory, so each distinct upstream document is rewritten once and requests arriving while it is being rewritten wait for that rewrite. New bytes from upstream are rewritten again. A PyPI simple page is cached per representation (HTML or JSON) and per set of versions filtered out of it. With version cooldown enabled, a cached rewrite expires when the first version it held back leaves cooldown, since versions it let through stay allowed and a new upstream version changes the document. A rewrite that held nothing back is kept for the shortest cooldown configured, at least a minute and at most a day. Cached documents carry an `ETag` of their own, and are dated like any other, so clients that revalidate with `If-None-Match` or `If-Modified-Since`, such as Composer, get `304 Not Modified`. The RubyGems compact index and Hex packages filtered by cooldown are kept the same way; they are never served gzip-compressed. Each cached document is kept gzip-compressed, when that makes it smaller, and served with `Content-Encoding: gzip` to clients that accept it, as npm, pnpm, Yarn and Composer do. A large packument is compressed once rather than on every request, by the proxy or by a reverse proxy in front of it. Only the compressed copy is kept, typically an eighth of the document, and it is decompressed for the clients that don't accept gzip. Documents rewritten outside the cache are served uncompressed. diff --git a/internal/handler/cargo.go b/internal/handler/cargo.go index 578c9ecd..0c265519 100644 --- a/internal/handler/cargo.go +++ b/internal/handler/cargo.go @@ -171,7 +171,7 @@ func (h *CargoHandler) applyCooldownFiltering(downstreamResponse http.ResponseWr cratePURL := canonicalPackagePURL("cargo", crate.Name) - if !h.proxy.Cooldown.IsAllowed("cargo", cratePURL, publishedAt) { + if !h.proxy.cooldown().IsAllowed("cargo", cratePURL, publishedAt) { h.proxy.Logger.Info("cooldown: filtering cargo version", "crate", crate.Name, "version", crate.Version, "published", crate.PublishTime) diff --git a/internal/handler/composer.go b/internal/handler/composer.go index 116d106e..ccb42bbb 100644 --- a/internal/handler/composer.go +++ b/internal/handler/composer.go @@ -394,7 +394,7 @@ func (h *ComposerHandler) shouldFilterVersion(packagePURL, packageName, version return false } - if !h.proxy.Cooldown.IsAllowed("composer", packagePURL, publishedAt) { + if !h.proxy.cooldown().IsAllowed("composer", packagePURL, publishedAt) { h.proxy.Logger.Info("cooldown: filtering composer version", "package", packageName, "version", version) return true @@ -731,7 +731,7 @@ func (h *ComposerHandler) serveCachedRelease(w http.ResponseWriter, r *http.Requ if err != nil || ver == nil || !ver.PublishedAt.Valid { return false } - if !h.proxy.Cooldown.IsAllowed("composer", canonicalPackagePURL("composer", packageName), ver.PublishedAt.Time) { + if !h.proxy.cooldown().IsAllowed("composer", canonicalPackagePURL("composer", packageName), ver.PublishedAt.Time) { h.proxy.Logger.Info("cooldown: withholding composer archive", "package", packageName, "version", version) http.Error(w, "version not found", http.StatusNotFound) diff --git a/internal/handler/conda.go b/internal/handler/conda.go index 76a43f2b..3cbaf937 100644 --- a/internal/handler/conda.go +++ b/internal/handler/conda.go @@ -493,7 +493,7 @@ func (f *repodataFilter) inCooldown(filename string, raw []byte) bool { return false } publishedAt := time.Unix(int64(entry.Timestamp)/condaTimestampDivisor, 0) - decision := f.h.proxy.Cooldown.Evaluate("conda", canonicalPackagePURL("conda", entry.Name), publishedAt, f.now) + decision := f.h.proxy.cooldown().Evaluate("conda", canonicalPackagePURL("conda", entry.Name), publishedAt, f.now) if decision.Allowed { return false } @@ -517,7 +517,7 @@ func (h *CondaHandler) recordInCooldown(filename, name, version string, timestam return false } publishedAt := time.Unix(int64(timestamp)/condaTimestampDivisor, 0) - if h.proxy.Cooldown.IsAllowed("conda", canonicalPackagePURL("conda", name), publishedAt) { + if h.proxy.cooldown().IsAllowed("conda", canonicalPackagePURL("conda", name), publishedAt) { return false } h.proxy.Logger.Info("cooldown: filtering conda package", diff --git a/internal/handler/cooldown_expiry.go b/internal/handler/cooldown_expiry.go new file mode 100644 index 00000000..1163bde9 --- /dev/null +++ b/internal/handler/cooldown_expiry.go @@ -0,0 +1,164 @@ +package handler + +import ( + "sync" + "time" + + "github.com/git-pkgs/cooldown" +) + +// A rewrite made with cooldown on stays right until a version it held back +// leaves cooldown: versions it let through stay allowed as time passes, and +// new versions upstream change the document and so the cache key. So the +// cached rewrite expires when the earliest version it withheld becomes +// available, instead of after a fixed time. +// +// Handlers ask cooldown through Proxy.cooldown, which tells every rewrite +// running at that moment when a withheld version becomes available. A +// rewrite may hear of versions another rewrite withheld at the same time; +// that only makes it expire earlier than it has to, never later. + +// maxCooldownRewriteTTL is the longest a rewrite made with cooldown on is +// served, even when it held nothing back. +const maxCooldownRewriteTTL = 24 * time.Hour + +// minCooldownRewriteTTL is the shortest cap on a rewrite made with cooldown +// on, however short the cooldown. +const minCooldownRewriteTTL = time.Minute + +// cooldownRewriteTTL returns the longest a rewrite made with cooldown on is +// served: the shortest cooldown configured, between a minute and a day. It +// only matters when a rewrite held nothing back, or held back a version +// whose availability is unknown. +func (p *Proxy) cooldownRewriteTTL() time.Duration { + ttl := p.ShortestCooldown + switch { + case ttl <= 0 || ttl > maxCooldownRewriteTTL: + return maxCooldownRewriteTTL + case ttl < minCooldownRewriteTTL: + return minCooldownRewriteTTL + } + return ttl +} + +// cooldownWatch holds the rewrites running with cooldown on. +type cooldownWatch struct { + mu sync.Mutex + active map[*cooldownRecorder]struct{} +} + +// cooldownRecorder collects when the versions withheld during one rewrite +// become available. +type cooldownRecorder struct { + // earliest is when the first withheld version becomes available, or + // zero if none was withheld. + earliest time.Time + // unknown is set when a version was withheld without a time it becomes + // available. + unknown bool +} + +// watchCooldown starts recording the versions cooldown holds back for a +// rewrite about to run. unwatchCooldown ends it. +func (p *Proxy) watchCooldown() *cooldownRecorder { + rec := &cooldownRecorder{} + w := &p.cooldownWatch + w.mu.Lock() + defer w.mu.Unlock() + if w.active == nil { + w.active = make(map[*cooldownRecorder]struct{}) + } + w.active[rec] = struct{}{} + return rec +} + +func (p *Proxy) unwatchCooldown(rec *cooldownRecorder) { + w := &p.cooldownWatch + w.mu.Lock() + defer w.mu.Unlock() + delete(w.active, rec) +} + +// cooldownExpiry returns when a rewrite that started at start and recorded +// into rec should expire: when the first version it held back leaves +// cooldown, and no later than cooldownRewriteTTL after start. +func (p *Proxy) cooldownExpiry(rec *cooldownRecorder, start time.Time) time.Time { + w := &p.cooldownWatch + w.mu.Lock() + earliest, unknown := rec.earliest, rec.unknown + w.mu.Unlock() + + limit := start.Add(p.cooldownRewriteTTL()) + if unknown || earliest.IsZero() || earliest.After(limit) { + return limit + } + return earliest +} + +// withheld tells the running rewrites that a version is held back until +// availableAt, zero when that is unknown. +func (p *Proxy) withheld(availableAt time.Time) { + w := &p.cooldownWatch + w.mu.Lock() + defer w.mu.Unlock() + for rec := range w.active { + switch { + case availableAt.IsZero(): + rec.unknown = true + case rec.earliest.IsZero() || availableAt.Before(rec.earliest): + rec.earliest = availableAt + } + } +} + +// cooldown returns the proxy's cooldown policy, telling the running rewrites +// about each version it holds back. Handlers use it rather than p.Cooldown. +func (p *Proxy) cooldown() CooldownPolicy { //nolint:ireturn // nil when cooldown is not configured, as p.Cooldown is + if p.Cooldown == nil { + return nil + } + return recordingCooldown{p: p} +} + +// recordingCooldown is the proxy's cooldown policy, reporting the versions it +// holds back to the running rewrites. +type recordingCooldown struct { + p *Proxy +} + +func (r recordingCooldown) IsAllowed(ecosystem, packagePURL string, publishedAt time.Time) bool { + if r.p.Cooldown.IsAllowed(ecosystem, packagePURL, publishedAt) { + return true + } + r.p.withheld(r.availableAt(ecosystem, packagePURL, publishedAt)) + return false +} + +func (r recordingCooldown) Evaluate(ecosystem, packagePURL string, publishedAt, evaluatedAt time.Time) cooldown.Decision { + d := r.p.Cooldown.Evaluate(ecosystem, packagePURL, publishedAt, evaluatedAt) + if !d.Allowed { + r.p.withheld(d.AvailableAt) + } + return d +} + +func (r recordingCooldown) For(ecosystem, packagePURL string) time.Duration { + return r.p.Cooldown.For(ecosystem, packagePURL) +} + +func (r recordingCooldown) Enabled() bool { + return r.p.Cooldown.Enabled() +} + +// availableAt is when a version IsAllowed held back leaves cooldown, or zero +// when that is unknown. +func (r recordingCooldown) availableAt(ecosystem, packagePURL string, publishedAt time.Time) time.Time { + if publishedAt.IsZero() { + return time.Time{} + } + d := r.p.Cooldown.For(ecosystem, packagePURL) + if d <= 0 { + return time.Time{} + } + return publishedAt.Add(d) +} diff --git a/internal/handler/cooldown_expiry_test.go b/internal/handler/cooldown_expiry_test.go new file mode 100644 index 00000000..fd558455 --- /dev/null +++ b/internal/handler/cooldown_expiry_test.go @@ -0,0 +1,178 @@ +package handler + +import ( + "context" + "net/http" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + "time" + + "github.com/git-pkgs/cooldown" +) + +// TestCachedRewrite_ExpiresWhenWithheldVersionIsAvailable checks that a +// rewrite which held a version back is cached until that version leaves +// cooldown, and no longer. +func TestCachedRewrite_ExpiresWhenWithheldVersionIsAvailable(t *testing.T) { + proxy, _, _, _ := setupTestProxy(t) + clock := &testClock{t: time.Now()} + proxy.now = clock.now + proxy.SetMetadataRewriteCacheSize(1 << 20) + proxy.Cooldown = &cooldown.Config{Default: "3d"} + purl := canonicalPackagePURL("npm", "left-pad") + // One version is 3d-5h old: it leaves cooldown in five hours. + published := clock.now().Add(-72*time.Hour + 5*time.Hour) + + var calls int + var rewrite keepingRewrite = func(b []byte) ([]byte, []string, error) { + calls++ + if proxy.cooldown().IsAllowed("npm", purl, published) { + return b, []string{"1.0.0", "1.1.0"}, nil + } + return b, []string{"1.0.0"}, nil + } + get := func() { + if _, err := proxy.cachedRewrite(context.Background(), "npm", "http://proxy", "left-pad", []byte("doc"), "", rewrite); err != nil { + t.Fatal(err) + } + } + + get() + clock.advance(5*time.Hour - time.Minute) + get() + if calls != 1 { + t.Errorf("rewrites before the withheld version is available = %d, want 1", calls) + } + clock.advance(time.Minute) + get() + if calls != 2 { + t.Errorf("rewrites once the withheld version is available = %d, want 2", calls) + } +} + +// TestCachedRewrite_NothingWithheldCachedForCap checks that a rewrite which +// held nothing back is cached for cooldownRewriteTTL. +func TestCachedRewrite_NothingWithheldCachedForCap(t *testing.T) { + proxy, _, _, _ := setupTestProxy(t) + clock := &testClock{t: time.Date(2026, 10, 10, 12, 0, 0, 0, time.UTC)} + proxy.now = clock.now + proxy.SetMetadataRewriteCacheSize(1 << 20) + proxy.Cooldown = &cooldown.Config{Default: "2h"} + proxy.ShortestCooldown = 2 * time.Hour + var r countingRewrite + + get := func() { + _, _ = proxy.cachedRewrite(context.Background(), "npm", "http://proxy", "left-pad", []byte("doc"), "", r.keeping) + } + get() + clock.advance(2*time.Hour - time.Second) + get() + if got := r.calls.Load(); got != 1 { + t.Errorf("rewrites within the cap = %d, want 1", got) + } + clock.advance(time.Second) + get() + if got := r.calls.Load(); got != 2 { + t.Errorf("rewrites after the cap = %d, want 2", got) + } +} + +// TestCooldownExpiry checks how one rewrite's recorded decisions set its +// expiry, including decisions made for other rewrites running at the same +// time, which may only make it expire earlier. +func TestCooldownExpiry(t *testing.T) { + p := &Proxy{Cooldown: &cooldown.Config{Default: "3d"}, ShortestCooldown: 72 * time.Hour} + // IsAllowed compares with the wall clock, so the times are relative to it. + start := time.Now() + limit := start.Add(24 * time.Hour) + purl := canonicalPackagePURL("npm", "left-pad") + + rec := p.watchCooldown() + if got := p.cooldownExpiry(rec, start); !got.Equal(limit) { + t.Errorf("nothing withheld: expiry = %v, want the cap %v", got, limit) + } + + other := p.watchCooldown() + p.cooldown().IsAllowed("npm", purl, start.Add(-60*time.Hour)) // available in 12h + p.unwatchCooldown(other) + p.cooldown().IsAllowed("npm", purl, start.Add(-66*time.Hour)) // available in 6h, after other ended + if got, want := p.cooldownExpiry(rec, start), start.Add(6*time.Hour); !got.Equal(want) { + t.Errorf("rec expiry = %v, want the earliest availability %v", got, want) + } + if got, want := p.cooldownExpiry(other, start), start.Add(12*time.Hour); !got.Equal(want) { + t.Errorf("other expiry = %v, want %v: decisions after it ended don't count", got, want) + } + + p.cooldown().Evaluate("npm", purl, start.Add(-71*time.Hour), start) // available in 1h + if got, want := p.cooldownExpiry(rec, start), start.Add(time.Hour); !got.Equal(want) { + t.Errorf("after Evaluate: expiry = %v, want %v", got, want) + } + p.unwatchCooldown(rec) + if len(p.cooldownWatch.active) != 0 { + t.Errorf("%d recorders left after unwatch", len(p.cooldownWatch.active)) + } +} + +// TestHandlersAskCooldownThroughRecorder checks that handlers make cooldown +// decisions through Proxy.cooldown. A decision made on Proxy.Cooldown +// directly would not reach the rewrites running, so a cached rewrite could +// keep hiding a version past its cooldown. +func TestHandlersAskCooldownThroughRecorder(t *testing.T) { + direct := regexp.MustCompile(`\.Cooldown\.(IsAllowed|Evaluate)\(|Cooldown:\s+\w+(\.\w+)*\.Cooldown\b`) + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + for _, f := range files { + if strings.HasSuffix(f, "_test.go") || f == "cooldown_expiry.go" { + continue + } + src, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + for i, line := range strings.Split(string(src), "\n") { + if direct.MatchString(line) { + t.Errorf("%s:%d asks Proxy.Cooldown directly; use Proxy.cooldown(): %s", f, i+1, strings.TrimSpace(line)) + } + } + } +} + +// TestHandlerRewriteExpiresWithWithheldVersion checks, through the handlers, +// that a cached rewrite expires when the version cooldown held back from it +// becomes available. +func TestHandlerRewriteExpiresWithWithheldVersion(t *testing.T) { + for _, c := range validatorCases { + t.Run(c.ecosystem, func(t *testing.T) { + u := cooldownUpstream(t) // the newest version was published 2h ago + proxy, _, clock := validatorProxyWithDB(t, u, time.Hour) + // Cooldown compares with the wall clock, so the proxy's clock + // follows it here. + clock.t = time.Now() + proxy.Cooldown = &cooldown.Config{Default: "3h"} + proxy.ShortestCooldown = 3 * time.Hour + handler := c.handler(proxy, u) + + if rec := serve(handler, c.path, nil); rec.Code != http.StatusOK { + t.Fatalf("status %d", rec.Code) + } + proxy.rewrites.mu.Lock() + defer proxy.rewrites.mu.Unlock() + if len(proxy.rewrites.entries) != 1 { + t.Fatalf("%d cached rewrites, want 1", len(proxy.rewrites.entries)) + } + for _, el := range proxy.rewrites.entries { + expires := el.Value.(*rewriteEntry).expires + // Published 2h ago with a 3h cooldown: available in an hour, + // to the second the publish time was given in. + if want := time.Now().Add(time.Hour); expires.Before(want.Add(-2*time.Second)) || expires.After(want.Add(time.Second)) { + t.Errorf("rewrite expires %v, want about %v when the withheld version is available", expires, want) + } + } + }) + } +} diff --git a/internal/handler/cooldown_times.go b/internal/handler/cooldown_times.go index 88532cd1..5d95d16a 100644 --- a/internal/handler/cooldown_times.go +++ b/internal/handler/cooldown_times.go @@ -126,7 +126,7 @@ func (d timedDocuments) withheld(p *Proxy, ecosystem, packagePURL, version strin if published.IsZero() { return false } - return !p.Cooldown.IsAllowed(ecosystem, packagePURL, published) + return !p.cooldown().IsAllowed(ecosystem, packagePURL, published) } // decodedMetadataBody returns res's body without its Content-Encoding. The diff --git a/internal/handler/go_cooldown.go b/internal/handler/go_cooldown.go index 90189d47..892e77b3 100644 --- a/internal/handler/go_cooldown.go +++ b/internal/handler/go_cooldown.go @@ -42,7 +42,7 @@ func (h *GoHandler) allowedAt(module string, published time.Time) bool { if published.IsZero() { return true } - return h.proxy.Cooldown.IsAllowed("golang", canonicalPackagePURL("golang", module), published) + return h.proxy.cooldown().IsAllowed("golang", canonicalPackagePURL("golang", module), published) } // writeGoMetadataError answers a failed upstream metadata fetch. diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 5693421a..1bc5ef7a 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -284,6 +284,10 @@ type Proxy struct { // the process started; see metadataDate. now func() time.Time started time.Time + + // cooldownWatch holds the rewrites recording which versions cooldown + // holds back; see cooldown_expiry.go. + cooldownWatch cooldownWatch } // CooldownPolicy supplies version-age filtering and package-specific durations. diff --git a/internal/handler/helm.go b/internal/handler/helm.go index 07e2f74e..fdc000ed 100644 --- a/internal/handler/helm.go +++ b/internal/handler/helm.go @@ -378,7 +378,7 @@ func (h *HelmHandler) buildChartLookup(upstreamURL string, body []byte) (*helmCh func (h *HelmHandler) chartOnCooldown(chartName string, created time.Time) bool { return !created.IsZero() && h.proxy.Cooldown != nil && h.proxy.Cooldown.Enabled() && - !h.proxy.Cooldown.IsAllowed(helmMetadataEcosystem, canonicalPackagePURL(helmMetadataEcosystem, chartName), created) + !h.proxy.cooldown().IsAllowed(helmMetadataEcosystem, canonicalPackagePURL(helmMetadataEcosystem, chartName), created) } type helmChartDownload struct { diff --git a/internal/handler/maven_cooldown.go b/internal/handler/maven_cooldown.go index 4705f700..4da839ee 100644 --- a/internal/handler/maven_cooldown.go +++ b/internal/handler/maven_cooldown.go @@ -222,7 +222,7 @@ func (h *MavenHandler) checkCooldown(w http.ResponseWriter, r *http.Request, sou http.Error(w, "failed to check cooldown", http.StatusBadGateway) return true } - if !h.proxy.Cooldown.IsAllowed("maven", mavenPackagePURL(group, artifact), published) { + if !h.proxy.cooldown().IsAllowed("maven", mavenPackagePURL(group, artifact), published) { h.proxy.Logger.Info("cooldown: withholding maven version", "group", group, "artifact", artifact, "version", version) http.Error(w, errMavenCooldown+": "+canonicalVersionPURL("maven", group+":"+artifact, version), http.StatusForbidden) @@ -307,7 +307,7 @@ func (h *MavenHandler) filterMetadata(ctx context.Context, source mavenSource, a packagePURL := mavenPackagePURL(group, artifact) // No version can be newer than the metadata listing it. if lastUpdated, err := time.Parse(mavenLastUpdatedLayout, doc.lastUpdated); err == nil && - h.proxy.Cooldown.IsAllowed("maven", packagePURL, lastUpdated) { + h.proxy.cooldown().IsAllowed("maven", packagePURL, lastUpdated) { return body, nil } @@ -324,7 +324,7 @@ func (h *MavenHandler) filterMetadata(ctx context.Context, source mavenSource, a if published.IsZero() { continue } - if h.proxy.Cooldown.IsAllowed("maven", packagePURL, published) { + if h.proxy.cooldown().IsAllowed("maven", packagePURL, published) { break } h.proxy.Logger.Info("cooldown: filtering maven version", "group", group, "artifact", artifact, "version", v) diff --git a/internal/handler/metadata_access_test.go b/internal/handler/metadata_access_test.go index 0a64b804..7b5e1cf4 100644 --- a/internal/handler/metadata_access_test.go +++ b/internal/handler/metadata_access_test.go @@ -143,7 +143,7 @@ func TestStoredRewrite_MarksAccess(t *testing.T) { body := []byte(`{}`) proxy.cacheMetadataBlob(t.Context(), "npm", "demo", metadataStoragePath("npm", "demo"), &upstreamMetadata{body: body}) key := rewriteCacheKey("npm", "http://proxy", "demo", body) - if _, err := proxy.rewrites.rewrite(t.Context(), key, body, 0, func(b []byte) ([]byte, error) { return b, nil }); err != nil { + if _, err := proxy.rewrites.rewrite(t.Context(), key, body, nil, func(b []byte) ([]byte, error) { return b, nil }); err != nil { t.Fatal(err) } old := ageMetadataAccess(t, db, "npm", "demo") diff --git a/internal/handler/metadata_gzip_test.go b/internal/handler/metadata_gzip_test.go index b5939c72..57d20cc1 100644 --- a/internal/handler/metadata_gzip_test.go +++ b/internal/handler/metadata_gzip_test.go @@ -184,7 +184,7 @@ func TestRewriteCache_KeepsGzipCopyOnlyWhenSmaller(t *testing.T) { for _, doc := range [][]byte{large, []byte("abc")} { key := rewriteCacheKey("npm", "http://proxy", string(doc[:3]), doc) - if _, err := c.rewrite(t.Context(), key, doc, 0, identity); err != nil { + if _, err := c.rewrite(t.Context(), key, doc, nil, identity); err != nil { t.Fatal(err) } } diff --git a/internal/handler/metadata_validators_test.go b/internal/handler/metadata_validators_test.go index f7c14753..340e3db6 100644 --- a/internal/handler/metadata_validators_test.go +++ b/internal/handler/metadata_validators_test.go @@ -500,7 +500,7 @@ func TestRewriteCacheETag(t *testing.T) { exclaim := func(b []byte) ([]byte, error) { return []byte(string(b) + "!"), nil } key := rewriteCacheKey("npm", "http://proxy", "demo", []byte("doc")) - m, err := c.rewrite(t.Context(), key, []byte("doc"), 0, exclaim) + m, err := c.rewrite(t.Context(), key, []byte("doc"), nil, exclaim) if err != nil || string(m.content()) != "doc!" { t.Fatalf("rewrite = %q, %v", m.content(), err) } diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 537c4b30..7263b3bb 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -467,7 +467,7 @@ func (h *NPMHandler) rewriteMetadataWithLocal(packageName string, body []byte, l func (h *NPMHandler) npmRewriteOptions(packageName string, local []npmLocalEntry) npm.Options { return npm.Options{ ProxyURL: h.proxyURL, - Cooldown: h.proxy.Cooldown, + Cooldown: h.proxy.cooldown(), DeniedVersions: h.proxy.Denylist.Versions(canonicalPackagePURL("npm", packageName)), EvaluatedAt: time.Now(), LocalVersions: local, @@ -685,7 +685,7 @@ func (h *NPMHandler) versionInCooldown(packageName, version string, metadata fun versionPURL := canonicalVersionPURL("npm", packageName, version) if !routed { if ver, err := h.proxy.DB.GetVersionByPURL(versionPURL); err == nil && ver != nil && ver.PublishedAt.Valid { - return !h.proxy.Cooldown.IsAllowed("npm", canonicalPackagePURL("npm", packageName), ver.PublishedAt.Time), nil + return !h.proxy.cooldown().IsAllowed("npm", canonicalPackagePURL("npm", packageName), ver.PublishedAt.Time), nil } } @@ -717,7 +717,7 @@ func (h *NPMHandler) versionInCooldown(packageName, version string, metadata fun } } - return !h.proxy.Cooldown.IsAllowed("npm", canonicalPackagePURL("npm", packageName), publishedAt), nil + return !h.proxy.cooldown().IsAllowed("npm", canonicalPackagePURL("npm", packageName), publishedAt), nil } // npmMetadataPath returns the package name and the encoded registry path. diff --git a/internal/handler/npm_rewrite_test.go b/internal/handler/npm_rewrite_test.go index b36b229c..9b8e35be 100644 --- a/internal/handler/npm_rewrite_test.go +++ b/internal/handler/npm_rewrite_test.go @@ -94,7 +94,7 @@ func TestNPMStoredRewriteSkippedWhenStale(t *testing.T) { body := []byte(`{}`) proxy.cacheMetadataBlob(t.Context(), "npm", "demo", metadataStoragePath("npm", "demo"), &upstreamMetadata{body: body}) key := rewriteCacheKey("npm", "http://proxy", "demo", body) - if _, err := proxy.rewrites.rewrite(t.Context(), key, body, 0, func(b []byte) ([]byte, error) { return b, nil }); err != nil { + if _, err := proxy.rewrites.rewrite(t.Context(), key, body, nil, func(b []byte) ([]byte, error) { return b, nil }); err != nil { t.Fatal(err) } if _, ok := proxy.storedRewrite("npm", "demo", "http://proxy", "demo"); !ok { @@ -129,7 +129,7 @@ func TestNPMStoredRewriteRecordsMetadataHit(t *testing.T) { body := []byte(`{"name":"demo"}`) proxy.cacheMetadataBlob(t.Context(), "npm", "hit-demo", metadataStoragePath("npm", "hit-demo"), &upstreamMetadata{body: body}) key := rewriteCacheKey("npm", "http://proxy", "hit-demo", body) - if _, err := proxy.rewrites.rewrite(t.Context(), key, body, 0, func(b []byte) ([]byte, error) { return b, nil }); err != nil { + if _, err := proxy.rewrites.rewrite(t.Context(), key, body, nil, func(b []byte) ([]byte, error) { return b, nil }); err != nil { t.Fatal(err) } diff --git a/internal/handler/nuget_cooldown.go b/internal/handler/nuget_cooldown.go index afb147d5..b9345c5f 100644 --- a/internal/handler/nuget_cooldown.go +++ b/internal/handler/nuget_cooldown.go @@ -204,7 +204,7 @@ func (h *NuGetHandler) nugetLeafAllowed(leaf map[string]any, id string) bool { if err != nil { return true } - return h.proxy.Cooldown.IsAllowed("nuget", canonicalPackagePURL("nuget", strings.ToLower(id)), when) + return h.proxy.cooldown().IsAllowed("nuget", canonicalPackagePURL("nuget", strings.ToLower(id)), when) } func nugetCatalogEntry(leaf map[string]any) map[string]any { diff --git a/internal/handler/pub.go b/internal/handler/pub.go index 3c812983..24fdd02c 100644 --- a/internal/handler/pub.go +++ b/internal/handler/pub.go @@ -247,7 +247,7 @@ func (h *PubHandler) shouldFilterVersion(packagePURL, name, version string, vmap return false } - if !h.proxy.Cooldown.IsAllowed("pub", packagePURL, publishedAt) { + if !h.proxy.cooldown().IsAllowed("pub", packagePURL, publishedAt) { h.proxy.Logger.Info("cooldown: filtering pub version", "package", name, "version", version) return true diff --git a/internal/handler/pypi.go b/internal/handler/pypi.go index fe018d8c..ebb30828 100644 --- a/internal/handler/pypi.go +++ b/internal/handler/pypi.go @@ -318,7 +318,7 @@ func (h *PyPIHandler) fetchFilteredVersions(r *http.Request, name string) (map[s filtered := make(map[string]bool) for version, publishedAt := range times { - if !publishedAt.IsZero() && !h.proxy.Cooldown.IsAllowed("pypi", packagePURL, publishedAt) { + if !publishedAt.IsZero() && !h.proxy.cooldown().IsAllowed("pypi", packagePURL, publishedAt) { filtered[version] = true } } @@ -611,7 +611,7 @@ func (h *PyPIHandler) shouldFilterRelease(packagePURL string, files any) bool { } publishedAt := h.newestUploadTime(filesArr) - return !publishedAt.IsZero() && !h.proxy.Cooldown.IsAllowed("pypi", packagePURL, publishedAt) + return !publishedAt.IsZero() && !h.proxy.cooldown().IsAllowed("pypi", packagePURL, publishedAt) } // versionInCooldown reports whether a version is still inside the cooldown @@ -636,7 +636,7 @@ func (h *PyPIHandler) versionInCooldown(r *http.Request, name, version string) ( return false, nil } h.storeUploadTime(name, version, publishedAt) - return !h.proxy.Cooldown.IsAllowed("pypi", canonicalPackagePURL("pypi", name), publishedAt), nil + return !h.proxy.cooldown().IsAllowed("pypi", canonicalPackagePURL("pypi", name), publishedAt), nil } // storeUploadTime records a release's newest upload time under the version @@ -675,7 +675,7 @@ func (h *PyPIHandler) storedCooldown(r *http.Request, name, version, filename st if err != nil || ver == nil || !ver.PublishedAt.Valid { return false, false } - if !h.proxy.Cooldown.IsAllowed("pypi", packagePURL, ver.PublishedAt.Time) { + if !h.proxy.cooldown().IsAllowed("pypi", packagePURL, ver.PublishedAt.Time) { return true, true } cached, err := h.proxy.HasCachedArtifact(r.Context(), "pypi", name, version, filename) diff --git a/internal/handler/rewrite_cache.go b/internal/handler/rewrite_cache.go index b2dc9921..fd455a78 100644 --- a/internal/handler/rewrite_cache.go +++ b/internal/handler/rewrite_cache.go @@ -35,8 +35,8 @@ import ( // depends on is fixed for the life of the process: the proxy URL is part of // the key, and the denylist is loaded at startup. The exception is cooldown, // which depends on the time of the request: with cooldown on, entries expire -// after Proxy.cooldownRewriteTTL, so a version is held back at most that long -// past its cooldown. +// when the first version they held back leaves cooldown (see +// cooldown_expiry.go). // // Each entry keeps the ETag of its output, so clients can revalidate their // copy without the output being hashed again on every request. Dates are not @@ -65,36 +65,6 @@ type rewriteEntry struct { expires time.Time } -// maxCooldownRewriteTTL is the longest a rewrite made with cooldown on is -// served. Versions leave cooldown as time passes, so a cached rewrite would -// otherwise keep hiding them. Cooldowns run to days, and a package manager -// that checks every lockfile entry on each install, as pnpm 12 does, would -// otherwise have the whole lockfile rewritten again on every install more -// than a minute after the last. -const maxCooldownRewriteTTL = time.Hour - -// minCooldownRewriteTTL is the shortest a rewrite made with cooldown on is -// served, however short the cooldown. -const minCooldownRewriteTTL = time.Minute - -// cooldownRewriteShare is the share of the shortest cooldown a rewrite made -// with cooldown on is served for, as a divisor. -const cooldownRewriteShare = 24 - -// cooldownRewriteTTL returns how long a rewrite made with cooldown on is -// served: a 24th of the shortest cooldown configured, so a version is held -// back at most about 4% past its cooldown, kept between a minute and an hour. -func (p *Proxy) cooldownRewriteTTL() time.Duration { - ttl := p.ShortestCooldown / cooldownRewriteShare - switch { - case ttl <= 0 || ttl > maxCooldownRewriteTTL: - return maxCooldownRewriteTTL - case ttl < minCooldownRewriteTTL: - return minCooldownRewriteTTL - } - return ttl -} - // size is how much of the cache's room the entry takes. func (e *rewriteEntry) size() int64 { return int64(len(e.plain) + len(e.gzipped)) @@ -279,7 +249,7 @@ func (c *rewriteCache) remove(el *list.Element) { // shared. A caller waiting on another's rewrite leaves when ctx ends; the // rewrite itself always runs to completion, so the result is cached for the // next request. -func (c *rewriteCache) rewrite(ctx context.Context, key string, in []byte, ttl time.Duration, rewrite func([]byte) ([]byte, error)) (rewrittenMetadata, error) { +func (c *rewriteCache) rewrite(ctx context.Context, key string, in []byte, expiry func(start time.Time) time.Time, rewrite func([]byte) ([]byte, error)) (rewrittenMetadata, error) { if c == nil { return rewriteWithETag(in, rewrite) } @@ -302,16 +272,17 @@ func (c *rewriteCache) rewrite(ctx context.Context, key string, in []byte, ttl t c.inFlight[key] = f c.mu.Unlock() - // The entry expires counting from before the rewrite, so it never - // outlives the time its cooldown decisions were made at by more than ttl. - var expires time.Time - if ttl > 0 { - expires = c.clock().Add(ttl) - } + // expiry is given the time from before the rewrite, so an entry never + // outlives the time its cooldown decisions were made at. + start := c.clock() defer func() { c.mu.Lock() delete(c.inFlight, key) if f.err == nil { + var expires time.Time + if expiry != nil { + expires = expiry(start) + } c.add(&rewriteEntry{key: key, rewrittenMetadata: f.m.compact(), expires: expires}) } c.mu.Unlock() @@ -370,7 +341,8 @@ func (c *rewriteCache) add(e *rewriteEntry) { // metadata cache stored it, and is empty otherwise; when known it keys the // cache, so the document isn't hashed again. // -// With cooldown on, the cached rewrite expires after p.cooldownRewriteTTL. Its +// With cooldown on, the cached rewrite expires when the first version it +// held back leaves cooldown, and after p.cooldownRewriteTTL at the latest. Its // ETag is the hash of its output, and it is dated like any other, so clients // revalidate with either validator and get the gzip copy. // @@ -390,11 +362,15 @@ func (p *Proxy) cachedRewrite(ctx context.Context, ecosystem, proxyURL, name str if digest == "" { key = rewriteCacheKey(ecosystem, proxyURL, name, in) } - var ttl time.Duration + var expiry func(time.Time) time.Time if cooldown { - ttl = p.cooldownRewriteTTL() + // Recording runs from before the rewrite until after it, so it + // hears of every version the rewrite holds back. + rec := p.watchCooldown() + defer p.unwatchCooldown(rec) + expiry = func(start time.Time) time.Time { return p.cooldownExpiry(rec, start) } } - m, err := p.rewrites.rewrite(ctx, key, in, ttl, rewriteOnly) + m, err := p.rewrites.rewrite(ctx, key, in, expiry, rewriteOnly) if err == nil { m.lastModified = p.metadataDate(ecosystem, proxyURL, name, m.etag) } diff --git a/internal/handler/rewrite_cache_test.go b/internal/handler/rewrite_cache_test.go index 7188634a..3f4c062d 100644 --- a/internal/handler/rewrite_cache_test.go +++ b/internal/handler/rewrite_cache_test.go @@ -44,7 +44,7 @@ func TestRewriteCache_RepeatServesCachedRewrite(t *testing.T) { key := rewriteCacheKey("npm", "http://proxy", "left-pad", []byte("doc")) for range 3 { - m, err := c.rewrite(context.Background(), key, []byte("doc"), 0, r.fn) + m, err := c.rewrite(context.Background(), key, []byte("doc"), nil, r.fn) if err != nil || string(m.content()) != "DOC" { t.Fatalf("rewrite = %q, %v", m.content(), err) } @@ -60,7 +60,7 @@ func TestRewriteCache_NewUpstreamBytesRewriteAgain(t *testing.T) { for _, doc := range []string{"v1", "v2"} { key := rewriteCacheKey("npm", "http://proxy", "left-pad", []byte(doc)) - m, err := c.rewrite(context.Background(), key, []byte(doc), 0, r.fn) + m, err := c.rewrite(context.Background(), key, []byte(doc), nil, r.fn) if err != nil || string(m.content()) != strings.ToUpper(doc) { t.Fatalf("rewrite(%s) = %q, %v", doc, m.content(), err) } @@ -82,7 +82,7 @@ func TestRewriteCache_ConcurrentRequestsShareRewrite(t *testing.T) { outs := make(chan string, n) run := func() { - m, err := c.rewrite(context.Background(), key, []byte("doc"), 0, r.fn) + m, err := c.rewrite(context.Background(), key, []byte("doc"), nil, r.fn) if err != nil { t.Error(err) } @@ -115,20 +115,20 @@ func TestRewriteCache_WaiterLeavingKeepsRewrite(t *testing.T) { first := make(chan error, 1) go func() { - _, err := c.rewrite(context.Background(), key, []byte("doc"), 0, r.fn) + _, err := c.rewrite(context.Background(), key, []byte("doc"), nil, r.fn) first <- err }() <-r.entered ctx, cancel := context.WithCancel(context.Background()) cancel() - if _, err := c.rewrite(ctx, key, []byte("doc"), 0, r.fn); !errors.Is(err, context.Canceled) { + if _, err := c.rewrite(ctx, key, []byte("doc"), nil, r.fn); !errors.Is(err, context.Canceled) { t.Errorf("waiter err = %v, want context.Canceled", err) } close(r.release) if err := <-first; err != nil { t.Fatal(err) } - if m, err := c.rewrite(context.Background(), key, []byte("doc"), 0, r.fn); err != nil || string(m.content()) != "DOC" { + if m, err := c.rewrite(context.Background(), key, []byte("doc"), nil, r.fn); err != nil || string(m.content()) != "DOC" { t.Errorf("cached rewrite = %q, %v", m.content(), err) } if got := r.calls.Load(); got != 1 { @@ -142,18 +142,18 @@ func TestRewriteCache_EvictsLeastRecentlyUsed(t *testing.T) { keyFor := func(doc string) string { return rewriteCacheKey("npm", "http://proxy", doc, []byte(doc)) } for _, doc := range []string{"aaa", "bbb"} { - _, _ = c.rewrite(context.Background(), keyFor(doc), []byte(doc), 0, r.fn) + _, _ = c.rewrite(context.Background(), keyFor(doc), []byte(doc), nil, r.fn) } - _, _ = c.rewrite(context.Background(), keyFor("aaa"), []byte("aaa"), 0, r.fn) // aaa is now the most recent - _, _ = c.rewrite(context.Background(), keyFor("ccc"), []byte("ccc"), 0, r.fn) // evicts bbb + _, _ = c.rewrite(context.Background(), keyFor("aaa"), []byte("aaa"), nil, r.fn) // aaa is now the most recent + _, _ = c.rewrite(context.Background(), keyFor("ccc"), []byte("ccc"), nil, r.fn) // evicts bbb before := r.calls.Load() - _, _ = c.rewrite(context.Background(), keyFor("aaa"), []byte("aaa"), 0, r.fn) - _, _ = c.rewrite(context.Background(), keyFor("ccc"), []byte("ccc"), 0, r.fn) + _, _ = c.rewrite(context.Background(), keyFor("aaa"), []byte("aaa"), nil, r.fn) + _, _ = c.rewrite(context.Background(), keyFor("ccc"), []byte("ccc"), nil, r.fn) if got := r.calls.Load() - before; got != 0 { t.Errorf("aaa and ccc rewritten %d times, want both still cached", got) } - _, _ = c.rewrite(context.Background(), keyFor("bbb"), []byte("bbb"), 0, r.fn) + _, _ = c.rewrite(context.Background(), keyFor("bbb"), []byte("bbb"), nil, r.fn) if got := r.calls.Load() - before; got != 1 { t.Errorf("bbb should have been evicted and rewritten") } @@ -164,7 +164,7 @@ func TestRewriteCache_OutputLargerThanCacheIsNotStored(t *testing.T) { var r countingRewrite key := rewriteCacheKey("npm", "http://proxy", "big", []byte("big")) - _, _ = c.rewrite(context.Background(), key, []byte("big"), 0, r.fn) + _, _ = c.rewrite(context.Background(), key, []byte("big"), nil, r.fn) if c.len() != 0 { t.Errorf("cached %d entries, want 0", c.len()) } @@ -177,7 +177,7 @@ func TestRewriteCache_ErrorsAreNotCached(t *testing.T) { key := rewriteCacheKey("npm", "http://proxy", "broken", []byte("doc")) for range 2 { - if _, err := c.rewrite(context.Background(), key, []byte("doc"), 0, failing); err == nil { + if _, err := c.rewrite(context.Background(), key, []byte("doc"), nil, failing); err == nil { t.Fatal("expected the rewrite error") } } @@ -232,7 +232,7 @@ func TestRewriteCacheExpiredEntryFreesRoom(t *testing.T) { clock := &testClock{t: time.Date(2026, 10, 10, 12, 0, 0, 0, time.UTC)} c.now = clock.now upper := func(b []byte) ([]byte, error) { return bytes.ToUpper(b), nil } - if _, err := c.rewrite(context.Background(), "k", []byte("doc"), time.Minute, upper); err != nil { + if _, err := c.rewrite(context.Background(), "k", []byte("doc"), func(start time.Time) time.Time { return start.Add(time.Minute) }, upper); err != nil { t.Fatal(err) } clock.advance(time.Minute) @@ -302,17 +302,16 @@ func (c *rewriteCache) len() int { return len(c.entries) } -// TestCooldownRewriteTTL checks that a rewrite filtered by cooldown is cached -// for a 24th of the shortest cooldown, between a minute and an hour. +// TestCooldownRewriteTTL checks that the cap on a rewrite filtered by +// cooldown is the shortest cooldown, between a minute and a day. func TestCooldownRewriteTTL(t *testing.T) { for _, tc := range []struct { shortest, want time.Duration }{ - {0, time.Hour}, - {72 * time.Hour, time.Hour}, - {24 * time.Hour, time.Hour}, - {2 * time.Hour, 5 * time.Minute}, - {10 * time.Minute, time.Minute}, + {0, 24 * time.Hour}, + {72 * time.Hour, 24 * time.Hour}, + {2 * time.Hour, 2 * time.Hour}, + {10 * time.Second, time.Minute}, } { p := &Proxy{ShortestCooldown: tc.shortest} if got := p.cooldownRewriteTTL(); got != tc.want { From 5b57994c09ffccc09d43a26eddf9b4e0e48578ba Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Fri, 30 Oct 2026 14:14:10 +0100 Subject: [PATCH 2/8] Stream npm packuments through storage when rewriting them Closes #525. With metadata caching and the rewrite cache on, a packument is streamed from upstream into storage and rewritten from there in two passes: the first collects the time map, dist-tags and version names, the second copies each kept version and rewrites its tarball URL. Neither the packument nor its rewrite is held in memory uncompressed, so metadata_max_size no longer limits packuments. Both passes check the stored bytes against the digest in the cache row, so a document replaced in between is not rewritten from mixed bytes. Documents the stream can't handle (abbreviated, no versions object) and rewrite errors fall back to the in-memory rewrite. --- docs/architecture.md | 2 +- docs/configuration.md | 2 +- internal/handler/conda.go | 3 +- internal/handler/handler.go | 87 ++++++--- internal/handler/metadata_stored.go | 207 ++++++++++++++++++++ internal/handler/npm.go | 80 ++++++++ internal/handler/npm_stream_test.go | 243 +++++++++++++++++++++++ internal/handler/rewrite_cache.go | 94 ++++++++- internal/metadata/npm/imports_test.go | 1 + internal/metadata/npm/stream.go | 270 ++++++++++++++++++++++++++ internal/metadata/npm/stream_test.go | 162 ++++++++++++++++ 11 files changed, 1111 insertions(+), 40 deletions(-) create mode 100644 internal/handler/metadata_stored.go create mode 100644 internal/handler/npm_stream_test.go create mode 100644 internal/metadata/npm/stream.go create mode 100644 internal/metadata/npm/stream_test.go diff --git a/docs/architecture.md b/docs/architecture.md index b6c654ff..c859f335 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -316,7 +316,7 @@ HTTP protocol handlers for each registry type. ### `internal/metadata/npm` -The npm metadata rewrite, separated from the handler so it can run where the proxy's infrastructure can't, such as WASI. It takes a packument's bytes and everything else as explicit inputs (proxy URL, cooldown policy, the time cooldown is evaluated at, denied versions, pushed versions) and returns the rewritten document with the versions it kept. It filters versions by cooldown and the denylist, moves `latest` off a withheld version, lists pushed versions in place of upstream ones, and points each tarball URL at the proxy, copying everything else byte for byte. It does no I/O and never reads the clock; callbacks report what it did, which the handler logs. It imports no database, storage, metrics, HTTP or logging packages, which a test checks. +The npm metadata rewrite, separated from the handler so it can run where the proxy's infrastructure can't, such as WASI. It takes a packument's bytes and everything else as explicit inputs (proxy URL, cooldown policy, the time cooldown is evaluated at, denied versions, pushed versions) and returns the rewritten document with the versions it kept. It filters versions by cooldown and the denylist, moves `latest` off a withheld version, lists pushed versions in place of upstream ones, and points each tarball URL at the proxy, copying everything else byte for byte. It does no I/O and never reads the clock; callbacks report what it did, which the handler logs. `ScanPackument` and `RewriteStream` do the same rewrite over a reader in two passes, the first collecting the time map, dist-tags and version names and the second copying each kept version, so the handler can rewrite a packument from storage without holding it in memory. It imports no database, storage, metrics, HTTP or logging packages, which a test checks. ### `internal/jsonscan` diff --git a/docs/configuration.md b/docs/configuration.md index 7f622071..6adfa383 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1400,7 +1400,7 @@ When upstream is unreachable and the cached entry is past its TTL, the proxy ser ### Metadata size limit -Upstream metadata responses are buffered in memory before being rewritten and served. `metadata_max_size` caps that buffer to protect against OOM from a misbehaving upstream. Some npm packages with thousands of versions (for example `renovate`) exceed the 100 MB default, so raise this if you see `metadata response exceeds size limit` in the logs. Conda `repodata.json` with cooldown enabled is filtered as it streams and isn't held in memory decompressed. With `cache_metadata` the limit applies to the compressed document as it does without cooldown; one over it is filtered as it streams from the upstream and not cached. +Upstream metadata responses are buffered in memory before being rewritten and served. `metadata_max_size` caps that buffer to protect against OOM from a misbehaving upstream. Some npm packages with thousands of versions (for example `renovate`) exceed the 100 MB default, so raise this if you see `metadata response exceeds size limit` in the logs. Conda `repodata.json` with cooldown enabled is filtered as it streams and isn't held in memory decompressed. With `cache_metadata` the limit applies to the compressed document as it does without cooldown; one over it is filtered as it streams from the upstream and not cached. With `cache_metadata` and the metadata rewrite cache on, an npm packument is streamed from the upstream into storage and rewritten from there in two passes, so it isn't held in memory and the limit doesn't apply to it; the rewritten copy is kept gzip-compressed. Requests for a single version (`//`) still read the packument into memory. ```yaml metadata_max_size: "100MB" # default diff --git a/internal/handler/conda.go b/internal/handler/conda.go index 3cbaf937..93ef0f02 100644 --- a/internal/handler/conda.go +++ b/internal/handler/conda.go @@ -409,7 +409,8 @@ func (h *CondaHandler) rememberRepodata(cacheKey, digest string, filter *repodat h.repodataStates[cacheKey] = repodataState{digest: digest, etag: etag, until: filter.until} } -// countingWriter records whether anything has reached the response yet. +// countingWriter counts the bytes written through it, such as whether +// anything has reached the response yet. type countingWriter struct { w io.Writer n int64 diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 1bc5ef7a..b2db3c55 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -1796,6 +1796,10 @@ type metadataResult struct { // fromCache is set when the body is a stored copy within its TTL, read // without asking upstream. fromCache bool + // entry is the cache row of the stored document, for a lookup that + // leaves the document in storage (see storedMetadataDocument). body is + // nil then. + entry *database.MetadataCacheEntry } // describedBy returns res with the digest that entry, the cache row its body @@ -1935,24 +1939,9 @@ type upstreamMetadata struct { // alongside for the caller to store and replay. An empty acceptEncoding leaves Go to negotiate and // decompress transparently. func (p *Proxy) fetchUpstreamMetadata(ctx context.Context, upstreamURL string, entry *database.MetadataCacheEntry, accept, acceptEncoding string) (*upstreamMetadata, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, upstreamURL, nil) - if err != nil { - return nil, fmt.Errorf("creating request: %w", err) - } - req.Header.Set("Accept", accept) - if acceptEncoding != "" { - req.Header.Set(headerAcceptEncoding, acceptEncoding) - } - - if entry != nil && entry.ETag.Valid { - req.Header.Set("If-None-Match", entry.ETag.String) - } else if entry != nil && entry.LastModified.Valid { - req.Header.Set("If-Modified-Since", entry.LastModified.Time.UTC().Format(http.TimeFormat)) - } - - resp, err := p.HTTPClient.Do(req) + resp, err := p.requestUpstreamMetadata(ctx, upstreamURL, entry, accept, acceptEncoding) if err != nil { - return nil, fmt.Errorf("fetching metadata: %w", err) + return nil, err } defer func() { _ = resp.Body.Close() }() @@ -1980,12 +1969,8 @@ func (p *Proxy) fetchUpstreamMetadata(ctx context.Context, upstreamURL string, e return meta, nil } - if resp.StatusCode == http.StatusNotFound { - body, _ := io.ReadAll(io.LimitReader(resp.Body, upstreamNotFoundBodyLimit)) - return nil, &upstreamNotFoundError{body: body} - } - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("upstream returned %d", resp.StatusCode) + if err := upstreamMetadataStatus(resp); err != nil { + return nil, err } // ContentLength is -1 when the transport decompresses the body, and @@ -2010,6 +1995,44 @@ func (p *Proxy) fetchUpstreamMetadata(ctx context.Context, upstreamURL string, e return meta, nil } +// requestUpstreamMetadata sends the request for a metadata document, +// revalidating entry when it is given. +func (p *Proxy) requestUpstreamMetadata(ctx context.Context, upstreamURL string, entry *database.MetadataCacheEntry, accept, acceptEncoding string) (*http.Response, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, upstreamURL, nil) + if err != nil { + return nil, fmt.Errorf("creating request: %w", err) + } + req.Header.Set("Accept", accept) + if acceptEncoding != "" { + req.Header.Set(headerAcceptEncoding, acceptEncoding) + } + + if entry != nil && entry.ETag.Valid { + req.Header.Set("If-None-Match", entry.ETag.String) + } else if entry != nil && entry.LastModified.Valid { + req.Header.Set("If-Modified-Since", entry.LastModified.Time.UTC().Format(http.TimeFormat)) + } + + resp, err := p.HTTPClient.Do(req) + if err != nil { + return nil, fmt.Errorf("fetching metadata: %w", err) + } + return resp, nil +} + +// upstreamMetadataStatus returns the error for a metadata response other +// than 200. Callers handle a 304 they asked for before. +func upstreamMetadataStatus(resp *http.Response) error { + switch resp.StatusCode { + case http.StatusOK: + return nil + case http.StatusNotFound: + body, _ := io.ReadAll(io.LimitReader(resp.Body, upstreamNotFoundBodyLimit)) + return &upstreamNotFoundError{body: body} + } + return fmt.Errorf("upstream returned %d", resp.StatusCode) +} + // refreshMetadataBlob records that upstream answered 304 for the cached // document meta was read from, by moving the row's fetched_at on. The bytes in // storage are unchanged, so they are neither written nor hashed again. It @@ -2037,14 +2060,22 @@ func (p *Proxy) refreshMetadataBlob(ecosystem, cacheKey string, meta *upstreamMe // database. It returns the digest it recorded for the bytes, or "" when they // were not cached. func (p *Proxy) cacheMetadataBlob(ctx context.Context, ecosystem, cacheKey, storagePath string, meta *upstreamMetadata) string { + digest, _ := p.storeMetadataBlob(ctx, ecosystem, cacheKey, storagePath, bytes.NewReader(meta.body), meta) + return digest +} + +// storeMetadataBlob is cacheMetadataBlob for a document read from body, +// which meta describes. It also returns the error that kept it from caching +// the document. +func (p *Proxy) storeMetadataBlob(ctx context.Context, ecosystem, cacheKey, storagePath string, body io.Reader, meta *upstreamMetadata) (string, error) { if p.DB == nil || p.Storage == nil { - return "" + return "", errMetadataNotStored } - size, hash, err := p.Storage.Store(ctx, storagePath, bytes.NewReader(meta.body)) + size, hash, err := p.Storage.Store(ctx, storagePath, body) if err != nil { p.Logger.Warn("failed to cache metadata", "ecosystem", ecosystem, "key", cacheKey, "error", err) - return "" + return "", err } var digest string if hash != "" { @@ -2074,9 +2105,9 @@ func (p *Proxy) cacheMetadataBlob(ctx context.Context, ecosystem, cacheKey, stor if delErr := p.Storage.Delete(ctx, storagePath); delErr != nil { p.Logger.Warn("failed to discard metadata blob", "ecosystem", ecosystem, "key", cacheKey, "error", delErr) } - return "" + return "", err } - return digest + return digest, nil } // currentMetadataEntry re-reads the metadata cache row and returns it, or diff --git a/internal/handler/metadata_stored.go b/internal/handler/metadata_stored.go new file mode 100644 index 00000000..e46239f0 --- /dev/null +++ b/internal/handler/metadata_stored.go @@ -0,0 +1,207 @@ +package handler + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "hash" + "io" + "net/http" + "time" + + "github.com/git-pkgs/proxy/internal/database" + "github.com/git-pkgs/proxy/internal/metrics" +) + +// A metadata document can be rewritten from storage instead of from memory: +// storedMetadataDocument streams it from upstream into storage, and the +// rewrite reads it back as often as it needs. Nothing holds the whole +// document, so its size is not limited by metadata_max_size or by memory. + +// errMetadataNotStored is returned when metadata is not cached in storage, so +// a document has to be read into memory. +var errMetadataNotStored = errors.New("metadata is not cached in storage") + +// errStoredMetadataChanged is returned when a stored document no longer has +// the digest its cache row records: a fetch replaced it while it was read. +var errStoredMetadataChanged = errors.New("stored metadata changed while it was read") + +// storedMetadataDocument returns the cache row of a metadata document stored +// uncompressed with its digest: the stored copy while it is within its TTL, +// or else the document fetched from upstream and stored, or the stored copy +// when upstream fails. The document is streamed from upstream into storage +// without being held in memory, so a fetch takes no metadata_max_size limit. +// Concurrent calls for one document share a fetch, and instances sharing the +// database take turns as for other metadata. +// +// It returns errMetadataNotStored when the proxy doesn't cache metadata. +func (p *Proxy) storedMetadataDocument(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string) (*database.MetadataCacheEntry, error) { + if !p.CacheMetadata || p.DB == nil || p.Storage == nil || p.MetadataTTL <= 0 { + return nil, errMetadataNotStored + } + if containsPathTraversal(cacheKey) { + return nil, fmt.Errorf("invalid cache key: %q", cacheKey) + } + if entry, fresh := p.storedMetadataEntryState(ctx, ecosystem, cacheKey); fresh { + metrics.RecordMetadataCacheHit(ecosystem, int(entry.Size.Int64)) + p.recordMetadataRead(entry) + return entry, nil + } + p.recordMetadataCacheMiss(ecosystem) + + key := metadataCoalesceKey(ecosystem, cacheKey, upstreamURL, accept, "", false) + "\x00stored" + res, _ := p.coalesceMetadata(ctx, key, func(fetchCtx context.Context) metadataResult { + entry, fresh := p.storedMetadataEntryState(fetchCtx, ecosystem, cacheKey) + if fresh { + return metadataResult{entry: entry} + } + release, outcome, err := p.awaitFetchLease(fetchCtx, metrics.CacheKindMetadata, key, func() bool { + entry, fresh = p.storedMetadataEntryState(fetchCtx, ecosystem, cacheKey) + return fresh + }) + if err != nil { + return metadataResult{err: err} + } + switch outcome { + case leaseStored: + return metadataResult{entry: entry} + case leaseFailed: + // Serve the stored copy, as the holder did, rather than trying + // the failing upstream again. + p.reloadUpstreamHolds() + if usableStoredMetadata(entry) { + return p.staleStoredMetadata(ecosystem, cacheKey, entry, errFetchFailedElsewhere) + } + case leaseFetch: + } + res := metadataResult{err: errSharedFetchAbandoned} + defer func() { release(res.err != nil || res.stale) }() + res = p.fetchStoredMetadata(fetchCtx, ecosystem, cacheKey, upstreamURL, accept, entry) + return res + }) + return res.entry, res.err +} + +// storedMetadataEntryState reads the cache row of a document and reports +// whether it is within its TTL and usable by storedMetadataDocument. +func (p *Proxy) storedMetadataEntryState(ctx context.Context, ecosystem, cacheKey string) (*database.MetadataCacheEntry, bool) { + entry, _ := p.DB.GetMetadataCache(ecosystem, cacheKey) + entry = p.migrateLegacyMetadataEntry(ctx, ecosystem, cacheKey, entry) + fresh := usableStoredMetadata(entry) && entry.FetchedAt.Valid && time.Since(entry.FetchedAt.Time) < p.MetadataTTL + return entry, fresh +} + +// usableStoredMetadata reports whether entry describes a document stored +// uncompressed with its digest, which a rewrite can read back and check. +func usableStoredMetadata(entry *database.MetadataCacheEntry) bool { + return entry != nil && entry.ContentDigest.Valid && entry.ContentEncoding.String == "" +} + +// fetchStoredMetadata fetches a document from upstream into storage, +// revalidating entry when there is one, and falls back to entry when +// upstream fails. +func (p *Proxy) fetchStoredMetadata(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, entry *database.MetadataCacheEntry) metadataResult { + revalidate := entry + if !usableStoredMetadata(entry) { + revalidate = nil + } + stored, err := p.storeUpstreamMetadata(ctx, ecosystem, cacheKey, upstreamURL, accept, revalidate) + if errors.Is(err, errStale304) { + stored, err = p.storeUpstreamMetadata(ctx, ecosystem, cacheKey, upstreamURL, accept, nil) + } + if err == nil { + return metadataResult{entry: stored} + } + if !usableStoredMetadata(entry) || errors.Is(err, ErrUpstreamNotFound) { + return metadataResult{err: err} + } + return p.staleStoredMetadata(ecosystem, cacheKey, entry, err) +} + +// staleStoredMetadata falls back to the stored document entry describes after +// the upstream fetch failed with err. +func (p *Proxy) staleStoredMetadata(ecosystem, cacheKey string, entry *database.MetadataCacheEntry, err error) metadataResult { + p.Logger.Warn("upstream metadata fetch failed, serving cached copy", + "ecosystem", ecosystem, "key", cacheKey, "error", err) + entry = p.currentMetadataEntry(ecosystem, cacheKey, entry) + if !usableStoredMetadata(entry) { + return metadataResult{err: fmt.Errorf("upstream failed and no usable cached metadata: %w", err)} + } + return metadataResult{entry: entry, stale: true} +} + +// storeUpstreamMetadata fetches a document and streams it into storage, and +// returns its new cache row. On a 304 for entry it only moves the row's +// fetched_at on; errStale304 means the row was replaced meanwhile and the +// caller should fetch without validators. +func (p *Proxy) storeUpstreamMetadata(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, entry *database.MetadataCacheEntry) (*database.MetadataCacheEntry, error) { + resp, err := p.requestUpstreamMetadata(ctx, upstreamURL, entry, accept, "") + if err != nil { + return nil, err + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusNotModified && entry != nil { + refreshed, err := p.DB.RefreshMetadataCache(ecosystem, cacheKey, entry.ContentDigest.String, time.Now()) + if err != nil || !refreshed { + return nil, errStale304 + } + return p.currentMetadataEntry(ecosystem, cacheKey, entry), nil + } + if err := upstreamMetadataStatus(resp); err != nil { + return nil, err + } + + meta := &upstreamMetadata{ + contentType: resp.Header.Get(headerContentType), + etag: resp.Header.Get(headerETag), + } + if meta.contentType == "" { + meta.contentType = contentTypeJSON + } + if lm := resp.Header.Get(headerLastModified); lm != "" { + meta.lastModified, _ = http.ParseTime(lm) + } + // Go decompresses the body, since no Accept-Encoding was set, so it is + // stored uncompressed. + if _, err := p.storeMetadataBlob(ctx, ecosystem, cacheKey, metadataStoragePath(ecosystem, cacheKey), resp.Body, meta); err != nil { + return nil, err + } + stored, err := p.DB.GetMetadataCache(ecosystem, cacheKey) + if err != nil || !usableStoredMetadata(stored) { + return nil, fmt.Errorf("reading the stored metadata row: %w", errors.Join(err, errMetadataNotStored)) + } + return stored, nil +} + +// openStoredMetadata opens the document entry describes. Reading it to the +// end fails with errStoredMetadataChanged when its bytes don't have the +// digest entry records. +func (p *Proxy) openStoredMetadata(ctx context.Context, entry *database.MetadataCacheEntry) (io.ReadCloser, error) { + r, err := p.Storage.Open(ctx, entry.StoragePath) + if err != nil { + return nil, err + } + return &digestReader{r: r, h: sha256.New(), want: entry.ContentDigest.String}, nil +} + +// digestReader checks at the end of its reader that what it read has the +// digest want, in the form the metadata cache records it. +type digestReader struct { + r io.ReadCloser + h hash.Hash + want string +} + +func (d *digestReader) Read(b []byte) (int, error) { + n, err := d.r.Read(b) + d.h.Write(b[:n]) + if errors.Is(err, io.EOF) && "sha256:"+hex.EncodeToString(d.h.Sum(nil)) != d.want { + return n, errStoredMetadataChanged + } + return n, err +} + +func (d *digestReader) Close() error { return d.r.Close() } diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 7263b3bb..99c12aea 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -255,6 +255,10 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques return } + if !versionLookup && h.serveStreamedPackument(w, r, packageName, upstreamURL, accept, metadataCacheKey) { + return + } + var validate func([]byte) error if !versionLookup { validate = h.packumentValidator() @@ -308,6 +312,82 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques serveRewrittenMetadata(w, r, rewritten) } +// serveStreamedPackument serves a packument rewritten from storage: the +// document is streamed from upstream into storage, then read twice, once to +// collect the time map and version names cooldown and the denylist decide +// on, and once to copy each kept version. registry.npmjs.org writes +// dist-tags before versions and time after them, so one pass can't do it. +// Neither the packument nor its rewrite is held in memory uncompressed, and +// packuments larger than metadata_max_size can be served. +// +// It reports false when it leaves the request to the in-memory rewrite: +// without metadata or rewrite caching, for a document it can't stream, such +// as an abbreviated packument where the full one is required, and when the +// rewrite fails. The document is stored by then, so the in-memory path reads +// it from storage. +func (h *NPMHandler) serveStreamedPackument(w http.ResponseWriter, r *http.Request, packageName, upstreamURL, accept, metadataCacheKey string) bool { + if h.proxy.rewrites == nil { + return false + } + entry, err := h.proxy.storedMetadataDocument(r.Context(), "npm", metadataCacheKey, upstreamURL, accept) + switch { + case err == nil: + case errors.Is(err, errMetadataNotStored): + return false + case errors.Is(err, ErrUpstreamNotFound): + JSONError(w, http.StatusNotFound, ErrPackageNotFound.Error()) + return true + case writeUpstreamHeld(w, err): + return true + default: + h.proxy.Logger.Error("failed to fetch npm metadata", "error", err) + JSONError(w, http.StatusBadGateway, "failed to fetch from upstream") + return true + } + + fullRequired := h.packumentValidator() != nil + options := h.npmRewriteOptions(packageName, nil) + rewritten, err := h.proxy.streamedRewrite(r.Context(), "npm", h.proxyURL, metadataCacheKey, entry, func(open func() (io.ReadCloser, error), out io.Writer) error { + var scan *npm.Scan + err := readStored(open, func(in io.Reader) (err error) { + scan, err = npm.ScanPackument(in) + return err + }) + if err != nil { + return err + } + if fullRequired && scan.Abbreviated() { + return npm.ErrNotStreamable + } + return readStored(open, func(in io.Reader) error { + _, err := npm.RewriteStream(packageName, scan, in, out, options) + return err + }) + }) + if err != nil { + if r.Context().Err() != nil { + return true // the client left while waiting on a shared rewrite + } + if !errors.Is(err, npm.ErrNotStreamable) { + h.proxy.Logger.Warn("failed to rewrite stored npm metadata, rewriting in memory", + "package", packageName, "error", err) + } + return false + } + serveRewrittenMetadata(w, r, rewritten) + return true +} + +// readStored opens a stored document and reads it with read. +func readStored(open func() (io.ReadCloser, error), read func(io.Reader) error) error { + in, err := open() + if err != nil { + return err + } + defer func() { _ = in.Close() }() + return read(in) +} + // packumentAccept returns the Accept header packuments are requested with. // It prefers the smaller abbreviated packument format but includes // application/json as a fallback so upstreams that reject the abbreviated type diff --git a/internal/handler/npm_stream_test.go b/internal/handler/npm_stream_test.go new file mode 100644 index 00000000..565f0627 --- /dev/null +++ b/internal/handler/npm_stream_test.go @@ -0,0 +1,243 @@ +package handler + +import ( + "bytes" + "compress/gzip" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/git-pkgs/cooldown" +) + +// bigPackument returns a packument laid out as registry.npmjs.org writes +// them, dist-tags before versions and time after, with n old versions padded +// so the document is large, and a latest published an hour ago. +func bigPackument(n int) string { + var b strings.Builder + b.WriteString(`{"_id":"big","name":"big","dist-tags":{"latest":"9.0.0","old":"1.0.0"},"versions":{`) + times := []string{`"created":"2015-01-01T00:00:00Z"`} + for i := range n { + v := fmt.Sprintf("1.0.%d", i) + if i == 0 { + v = "1.0.0" + } + fmt.Fprintf(&b, `%q:{"name":"big","version":%q,"readme":%q,"dist":{"tarball":"https://registry.npmjs.org/big/-/big-%s.tgz"}},`, + v, v, strings.Repeat("x", 512), v) + times = append(times, fmt.Sprintf(`%q:"2016-01-01T00:%02d:%02dZ"`, v, i/60%60, i%60)) + } + recent := time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) + b.WriteString(`"9.0.0":{"name":"big","version":"9.0.0","dist":{"tarball":"https://registry.npmjs.org/big/-/big-9.0.0.tgz"}}},`) + times = append(times, `"9.0.0":"`+recent+`"`) + b.WriteString(`"time":{` + strings.Join(times, ",") + `},"readme":"big"}`) + return b.String() +} + +type streamUpstream struct { + *httptest.Server + body atomic.Value // string + fail atomic.Bool + requests atomic.Int32 +} + +func newStreamUpstream(t *testing.T, body string) *streamUpstream { + t.Helper() + u := &streamUpstream{} + u.body.Store(body) + u.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + u.requests.Add(1) + switch { + case u.fail.Load(): + http.Error(w, "down", http.StatusBadGateway) + case r.URL.Path == "/big" && r.Header.Get("If-None-Match") == `"v1"`: + w.WriteHeader(http.StatusNotModified) + case r.URL.Path == "/big": + w.Header().Set("Content-Type", "application/json") + w.Header().Set("ETag", `"v1"`) + _, _ = io.WriteString(w, u.body.Load().(string)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(u.Close) + return u +} + +func streamProxy(t *testing.T, u *streamUpstream) (*Proxy, *mockStorage, http.Handler) { + t.Helper() + proxy, _, store, _ := setupTestProxy(t) + proxy.HTTPClient = u.Client() + proxy.CacheMetadata = true + proxy.MetadataTTL = time.Hour + proxy.SetMetadataRewriteCacheSize(64 << 20) + proxy.Cooldown = &cooldown.Config{Default: "3d"} + return proxy, store, NewNPMHandler(proxy, "http://proxy.example", u.URL).Routes() +} + +type packumentDoc struct { + DistTags map[string]string `json:"dist-tags"` + Versions map[string]json.RawMessage `json:"versions"` + Time map[string]string `json:"time"` +} + +func decodeStreamed(t *testing.T, rec *httptest.ResponseRecorder) packumentDoc { + t.Helper() + if rec.Code != http.StatusOK { + t.Fatalf("status %d: %s", rec.Code, rec.Body.String()) + } + body := rec.Body.Bytes() + if rec.Header().Get("Content-Encoding") == "gzip" { + zr, err := gzip.NewReader(bytes.NewReader(body)) + if err != nil { + t.Fatal(err) + } + if body, err = io.ReadAll(zr); err != nil { + t.Fatal(err) + } + } + var doc packumentDoc + if err := json.Unmarshal(body, &doc); err != nil { + t.Fatalf("response is not JSON: %v", err) + } + return doc +} + +// TestNPMStreamedPackument requests a packument larger than +// metadata_max_size with cooldown on, its time map after its versions and +// its latest inside cooldown. The in-memory rewrite can't read it at all. +func TestNPMStreamedPackument(t *testing.T) { + const versions = 400 + body := bigPackument(versions) + u := newStreamUpstream(t, body) + proxy, _, handler := streamProxy(t, u) + proxy.MetadataMaxSize = int64(len(body) / 4) + + for _, header := range []http.Header{nil, {"Accept-Encoding": {"gzip"}}} { + doc := decodeStreamed(t, serve(handler, "/big", header)) + if len(doc.Versions) != versions { + t.Errorf("%d versions, want %d without 9.0.0", len(doc.Versions), versions) + } + if _, ok := doc.Versions["9.0.0"]; ok { + t.Error("9.0.0 is listed although it is inside cooldown") + } + if _, ok := doc.Time["9.0.0"]; ok { + t.Error("time still lists 9.0.0") + } + if doc.DistTags["latest"] != fmt.Sprintf("1.0.%d", versions-1) || doc.DistTags["old"] != "1.0.0" { + t.Errorf("dist-tags = %v, want latest moved to the newest version left", doc.DistTags) + } + if !strings.Contains(string(doc.Versions["1.0.0"]), `"tarball":"http://proxy.example/npm/big/-/big-1.0.0.tgz"`) { + t.Errorf("tarball not rewritten: %s", doc.Versions["1.0.0"]) + } + } + if n := u.requests.Load(); n != 1 { + t.Errorf("upstream asked %d times, want once", n) + } + + // Without the rewrite cache the in-memory path runs, which the size + // limit stops. + proxy.SetMetadataRewriteCacheSize(0) + if rec := serve(handler, "/big", nil); rec.Code == http.StatusOK { + t.Error("in-memory rewrite served a packument over metadata_max_size; the test no longer covers the stream") + } +} + +// TestNPMStreamedPackumentMatchesInMemory checks that both paths serve the +// same document and ETag. +func TestNPMStreamedPackumentMatchesInMemory(t *testing.T) { + body := bigPackument(50) + u := newStreamUpstream(t, body) + _, _, streamed := streamProxy(t, u) + inMemory, _, handler := streamProxy(t, u) + inMemory.CacheMetadata = false + + a, b := serve(streamed, "/big", nil), serve(handler, "/big", nil) + if a.Code != http.StatusOK || b.Code != http.StatusOK { + t.Fatalf("status %d and %d", a.Code, b.Code) + } + var ca, cb bytes.Buffer + if err := json.Compact(&ca, a.Body.Bytes()); err != nil { + t.Fatal(err) + } + if err := json.Compact(&cb, b.Body.Bytes()); err != nil { + t.Fatal(err) + } + if !bytes.Equal(ca.Bytes(), cb.Bytes()) { + t.Error("streamed and in-memory rewrites differ") + } +} + +// TestNPMStreamedPackumentStale serves the stored copy when upstream fails +// after the TTL, and 404 for a package upstream doesn't have. +func TestNPMStreamedPackumentStale(t *testing.T) { + u := newStreamUpstream(t, bigPackument(10)) + proxy, _, handler := streamProxy(t, u) + decodeStreamed(t, serve(handler, "/big", nil)) + + proxy.MetadataTTL = time.Nanosecond + proxy.SetMetadataRewriteCacheSize(64 << 20) // drop the cached rewrite + u.fail.Store(true) + if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != 10 { + t.Errorf("stale copy has %d versions, want 10", len(doc.Versions)) + } + + u.fail.Store(false) + if rec := serve(handler, "/missing", nil); rec.Code != http.StatusNotFound { + t.Errorf("missing package: status %d, want 404", rec.Code) + } +} + +// TestNPMStreamedPackumentChangedInStorage checks that a document replaced +// in storage between the two passes isn't rewritten from mixed bytes. +func TestNPMStreamedPackumentChangedInStorage(t *testing.T) { + u := newStreamUpstream(t, bigPackument(10)) + proxy, store, _ := streamProxy(t, u) + entry, err := proxy.storedMetadataDocument(context.Background(), "npm", "big", u.URL+"/big", contentTypeJSON) + if err != nil { + t.Fatal(err) + } + store.mu.Lock() + store.files[entry.StoragePath] = []byte(bigPackument(11)) + store.mu.Unlock() + + r, err := proxy.openStoredMetadata(context.Background(), entry) + if err != nil { + t.Fatal(err) + } + defer func() { _ = r.Close() }() + if _, err := io.ReadAll(r); !errors.Is(err, errStoredMetadataChanged) { + t.Errorf("read error %v, want errStoredMetadataChanged", err) + } +} + +// TestNPMStreamedPackumentRevalidates checks that a 304 after the TTL keeps +// the stored document and its cached rewrite, without storing it again. +func TestNPMStreamedPackumentRevalidates(t *testing.T) { + u := newStreamUpstream(t, bigPackument(10)) + proxy, store, handler := streamProxy(t, u) + first := serve(handler, "/big", nil) + decodeStreamed(t, first) + + proxy.MetadataTTL = time.Nanosecond + second := serve(handler, "/big", nil) + decodeStreamed(t, second) + if second.Header().Get("ETag") != first.Header().Get("ETag") { + t.Errorf("ETag changed from %s to %s", first.Header().Get("ETag"), second.Header().Get("ETag")) + } + if n := u.requests.Load(); n != 2 { + t.Errorf("upstream asked %d times, want 2", n) + } + store.mu.Lock() + defer store.mu.Unlock() + if store.stores != 1 { + t.Errorf("document stored %d times, want once", store.stores) + } +} diff --git a/internal/handler/rewrite_cache.go b/internal/handler/rewrite_cache.go index fd455a78..a624dbac 100644 --- a/internal/handler/rewrite_cache.go +++ b/internal/handler/rewrite_cache.go @@ -18,6 +18,7 @@ import ( "sync" "time" + "github.com/git-pkgs/proxy/internal/database" "github.com/git-pkgs/proxy/internal/metrics" ) @@ -253,6 +254,18 @@ func (c *rewriteCache) rewrite(ctx context.Context, key string, in []byte, expir if c == nil { return rewriteWithETag(in, rewrite) } + return c.produce(ctx, key, expiry, func() (rewrittenMetadata, error) { + m, err := rewriteWithETag(in, rewrite) + if err == nil { + m.gzipped = gzipIfSmaller(m.plain) + } + return m, err + }) +} + +// produce is rewrite for a document that build makes with its validators +// and gzip copy. +func (c *rewriteCache) produce(ctx context.Context, key string, expiry func(start time.Time) time.Time, build func() (rewrittenMetadata, error)) (rewrittenMetadata, error) { c.mu.Lock() if e, ok := c.lookup(key, c.clock()); ok { @@ -288,10 +301,7 @@ func (c *rewriteCache) rewrite(ctx context.Context, key string, in []byte, expir c.mu.Unlock() close(f.done) }() - f.m, f.err = rewriteWithETag(in, rewrite) - if f.err == nil { - f.m.gzipped = gzipIfSmaller(f.m.plain) - } + f.m, f.err = build() return f.m, f.err } @@ -391,6 +401,60 @@ func (p *Proxy) cachedRewrite(ctx context.Context, ecosystem, proxyURL, name str return m, err } +// streamedRewrite rewrites a document stored in storage, through the +// proxy's rewrite cache, without holding the document or its rewrite in +// memory uncompressed. rewrite is given a way to open the stored document, +// as often as it needs to read it, and writes the rewritten document to w. +// The result is cached under the same key cachedRewrite uses for the +// document, and expires the same way with cooldown on. +// +// It returns errMetadataNotStored without the rewrite cache, which the +// result has to be kept in. +func (p *Proxy) streamedRewrite(ctx context.Context, ecosystem, proxyURL, name string, entry *database.MetadataCacheEntry, rewrite func(open func() (io.ReadCloser, error), w io.Writer) error) (rewrittenMetadata, error) { + if p.rewrites == nil { + return rewrittenMetadata{}, errMetadataNotStored + } + var expiry func(time.Time) time.Time + if p.cooldownEnabled() { + rec := p.watchCooldown() + defer p.unwatchCooldown(rec) + expiry = func(start time.Time) time.Time { return p.cooldownExpiry(rec, start) } + } + key := rewriteCacheKeyForDigest(ecosystem, proxyURL, name, entry.ContentDigest.String) + m, err := p.rewrites.produce(ctx, key, expiry, func() (rewrittenMetadata, error) { + // The rewrite runs to completion for the cache even when the + // caller leaves. + readCtx := context.WithoutCancel(ctx) + return gzipRewrite(func(w io.Writer) error { + return rewrite(func() (io.ReadCloser, error) { return p.openStoredMetadata(readCtx, entry) }, w) + }) + }) + if err == nil { + m.lastModified = p.metadataDate(ecosystem, proxyURL, name, m.etag) + } + return m, err +} + +// gzipRewrite runs write and keeps what it writes gzip-compressed only, with +// the ETag and length of the uncompressed output. +func gzipRewrite(write func(w io.Writer) error) (rewrittenMetadata, error) { + var buf bytes.Buffer + zw := gzip.NewWriter(&buf) + sum := sha256.New() + n := &countingWriter{w: io.Discard} + if err := write(io.MultiWriter(zw, sum, n)); err != nil { + return rewrittenMetadata{}, err + } + if err := zw.Close(); err != nil { + return rewrittenMetadata{}, err + } + return rewrittenMetadata{ + metadataValidators: metadataValidators{etag: `"` + hex.EncodeToString(sum.Sum(nil)) + `"`}, + gzipped: buf.Bytes(), + size: int(n.n), + }, nil +} + // uncachedRewrite gives a rewrite made outside the cache its validators: an // ETag of the output itself and, as cachedRewrite does, a date that replicas // agree on. @@ -501,16 +565,28 @@ func serveRewrittenMetadataAs(w http.ResponseWriter, r *http.Request, m rewritte w.WriteHeader(http.StatusNotModified) return } - body := m.gzipped - if !compressed { - body = m.content() - } w.Header().Set(headerContentType, contentType) if compressed { w.Header().Set(headerContentEncoding, "gzip") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(m.gzipped) + return + } + if m.plain == nil && m.gzipped != nil { + // Decompressed as it is sent, rather than into a copy of the + // whole document. + zr, err := gzip.NewReader(bytes.NewReader(m.gzipped)) + if err != nil { + http.Error(w, "failed to read cached metadata", http.StatusInternalServerError) + return + } + w.Header().Set(headerContentLength, strconv.Itoa(m.size)) + w.WriteHeader(http.StatusOK) + _, _ = io.Copy(w, zr) + return } w.WriteHeader(http.StatusOK) - _, _ = w.Write(body) + _, _ = w.Write(m.plain) } // ensureVary adds field to the Vary header unless it is listed already. diff --git a/internal/metadata/npm/imports_test.go b/internal/metadata/npm/imports_test.go index 0f04bc18..19f828c1 100644 --- a/internal/metadata/npm/imports_test.go +++ b/internal/metadata/npm/imports_test.go @@ -23,6 +23,7 @@ func TestPortableImports(t *testing.T) { "github.com/Masterminds/semver/v3", "github.com/git-pkgs/cooldown", "github.com/git-pkgs/proxy/internal/jsonscan", + "github.com/git-pkgs/proxy/internal/jsonstream", "github.com/git-pkgs/proxy/internal/metadata/npm", "github.com/git-pkgs/proxy/internal/packageurl", "github.com/git-pkgs/purl", diff --git a/internal/metadata/npm/stream.go b/internal/metadata/npm/stream.go new file mode 100644 index 00000000..cc70c52b --- /dev/null +++ b/internal/metadata/npm/stream.go @@ -0,0 +1,270 @@ +package npm + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "maps" + "reflect" + "slices" + + "github.com/git-pkgs/proxy/internal/jsonstream" +) + +// ErrNotStreamable is returned by RewriteStream for a document it leaves to +// Rewrite: one without a versions object, such as a single version's +// metadata, or a rewrite that lists pushed versions. +var ErrNotStreamable = errors.New("npm metadata can't be rewritten as a stream") + +// Scan is what a first pass over a packument learns: the version names, and +// the time map and dist-tags the filters work on. It holds no version +// entries, so it stays small whatever the size of the packument. +type Scan struct { + versions []string + versionsObject bool + time any + distTags any + // occurrences counts each filtered key, so the second pass can drop the + // earlier duplicates, as Rewrite does. + occurrences map[string]int + hasModified bool +} + +// Abbreviated reports whether the scanned document is an abbreviated +// packument, as the package function Abbreviated does for a document in +// memory. +func (s *Scan) Abbreviated() bool { + return s.hasModified && s.occurrences["time"] == 0 +} + +// ScanPackument reads a packument once, keeping only what RewriteStream +// needs to decide which versions remain. Version entries are read one at a +// time and dropped. +func ScanPackument(r io.Reader) (*Scan, error) { + s := &Scan{occurrences: map[string]int{}} + dec := json.NewDecoder(r) + if err := expectDelim(dec, '{'); err != nil { + return nil, err + } + for dec.More() { + key, err := objectKey(dec) + if err != nil { + return nil, err + } + switch key { + case "versions": + s.occurrences[key]++ + s.versions, s.versionsObject, err = objectKeys(dec) + case "time": + s.occurrences[key]++ + s.time = nil + err = dec.Decode(&s.time) + case "dist-tags": + s.occurrences[key]++ + s.distTags = nil + err = dec.Decode(&s.distTags) + default: + if key == "modified" { + s.hasModified = true + } + err = skipValue(dec) + } + if err != nil { + return nil, err + } + } + if err := expectDelim(dec, '}'); err != nil { + return nil, err + } + if _, err := dec.Token(); err != io.EOF { + if err == nil { + err = errors.New("unexpected data after the top-level object") + } + return nil, err + } + return s, nil +} + +// RewriteStream is Rewrite for a packument too large to hold in memory. scan +// is the result of ScanPackument over the same bytes r reads again; the +// caller checks they are the same. The rewritten document is written to w +// with the bytes of untouched values copied as they are, and its output is +// the same as Rewrite's for the same document and options. It returns the +// versions it kept. For a document without a versions object, or options +// listing pushed versions, it returns ErrNotStreamable before writing. +func RewriteStream(packageName string, scan *Scan, r io.Reader, w io.Writer, options Options) ([]string, error) { + if !scan.versionsObject || len(options.LocalVersions) > 0 { + return nil, ErrNotStreamable + } + h := &transformer{options} + + versions := make(map[string]any, len(scan.versions)) + for _, v := range scan.versions { + versions[v] = nil + } + metadata := map[string]any{} + timeMap := scan.objectValue(metadata, "time", scan.time) + distTags := scan.objectValue(metadata, "dist-tags", scan.distTags) + timeEntries := len(timeMap) + originalTags := maps.Clone(distTags) + + h.applyCooldownFiltering(metadata, versions, packageName) + h.applyDenylistFiltering(metadata, versions) + + sw := &streamWriter{ + transformer: h, + packageName: packageName, + scan: scan, + versions: versions, + timeMap: timeMap, + distTags: distTags, + timeChanged: timeMap != nil && len(timeMap) != timeEntries, + tagsChanged: distTags != nil && !reflect.DeepEqual(distTags, originalTags), + seen: map[string]int{}, + } + if err := jsonstream.CopyObject(json.NewDecoder(r), w, sw.member); err != nil { + return nil, err + } + h.tarballsRewritten(sw.rewritten) + return slices.Collect(maps.Keys(versions)), nil +} + +// streamWriter writes the second pass of RewriteStream from the filtered +// state the first pass left. +type streamWriter struct { + *transformer + packageName string + scan *Scan + versions map[string]any + timeMap, distTags map[string]any + timeChanged, tagsChanged bool + seen map[string]int + entry bytes.Buffer + rewritten int +} + +// member writes one top-level member. Members the filters don't touch are +// left unconsumed, so CopyObject copies them as they are. +func (s *streamWriter) member(m *jsonstream.Member) error { + total, filtered := s.scan.occurrences[m.Key] + if !filtered { + return nil + } + s.seen[m.Key]++ + if s.seen[m.Key] < total { + _, err := m.Raw() // an earlier duplicate, dropped + return err + } + switch { + case m.Key == "versions": + return m.Object(s.version) + case m.Key == "time" && s.timeChanged: + return m.Object(s.time) + case m.Key == "dist-tags" && s.tagsChanged: + if _, err := m.Raw(); err != nil { + return err + } + encoded, err := json.Marshal(s.distTags) + if err != nil { + return err + } + return m.Write(encoded) + } + return nil +} + +// version writes a kept version with its tarball pointing at the proxy. +func (s *streamWriter) version(v *jsonstream.Member) error { + raw, err := v.Raw() + if err != nil { + return err + } + if _, keep := s.versions[v.Key]; !keep { + return nil + } + s.entry.Reset() + if s.writeNPMVersion(&s.entry, s.packageName, v.Key, raw) { + s.rewritten++ + } + return v.Write(s.entry.Bytes()) +} + +// time writes a time entry the filters kept. +func (s *streamWriter) time(t *jsonstream.Member) error { + if _, keep := s.timeMap[t.Key]; keep { + return t.Copy() + } + _, err := t.Raw() + return err +} + +// objectValue records value under name in metadata, as Rewrite's +// decodeObject does, and returns it when it is an object. +func (s *Scan) objectValue(metadata map[string]any, name string, value any) map[string]any { + if s.occurrences[name] == 0 { + return nil + } + metadata[name] = value + object, _ := value.(map[string]any) + return object +} + +func expectDelim(dec *json.Decoder, want json.Delim) error { + tok, err := dec.Token() + if err != nil { + return err + } + if tok != want { + return fmt.Errorf("npm metadata: expected %v, got %v", want, tok) + } + return nil +} + +func objectKey(dec *json.Decoder) (string, error) { + tok, err := dec.Token() + if err != nil { + return "", err + } + key, ok := tok.(string) + if !ok { + return "", fmt.Errorf("npm metadata: object key is %T, not a string", tok) + } + return key, nil +} + +// objectKeys reads a value and returns its keys when it is an object. +func objectKeys(dec *json.Decoder) ([]string, bool, error) { + tok, err := dec.Token() + if err != nil { + return nil, false, err + } + delim, ok := tok.(json.Delim) + if !ok { + return nil, false, nil // a scalar, read whole by Token + } + var keys []string + for dec.More() { + if delim == '{' { + key, err := objectKey(dec) + if err != nil { + return nil, false, err + } + keys = append(keys, key) + } + if err := skipValue(dec); err != nil { + return nil, false, err + } + } + if _, err := dec.Token(); err != nil { + return nil, false, err + } + return keys, delim == '{', nil +} + +// skipValue reads the next value without keeping it. +func skipValue(dec *json.Decoder) error { + var raw json.RawMessage + return dec.Decode(&raw) +} diff --git a/internal/metadata/npm/stream_test.go b/internal/metadata/npm/stream_test.go new file mode 100644 index 00000000..ef2c9542 --- /dev/null +++ b/internal/metadata/npm/stream_test.go @@ -0,0 +1,162 @@ +package npm + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "slices" + "strings" + "testing" + "time" + + "github.com/git-pkgs/cooldown" +) + +// rewriteStream runs both passes of RewriteStream over body. +func rewriteStream(packageName string, body []byte, options Options) ([]byte, []string, error) { + scan, err := ScanPackument(bytes.NewReader(body)) + if err != nil { + return nil, nil, err + } + var out bytes.Buffer + kept, err := RewriteStream(packageName, scan, bytes.NewReader(body), &out, options) + return out.Bytes(), kept, err +} + +// TestRewriteStreamMatchesRewrite checks that the streamed rewrite writes the +// same bytes and keeps the same versions as Rewrite, over documents with the +// filtered members in either order, duplicates, and values Rewrite copies +// verbatim. +func TestRewriteStreamMatchesRewrite(t *testing.T) { + now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) + ts := func(age time.Duration) string { return now.Add(-age).Format(time.RFC3339) } + version := func(v string) string { + return fmt.Sprintf(`%q:{"version":%q,"dist":{"tarball":"https://registry.npmjs.org/demo/-/demo-%s.tgz"}}`, v, v, v) + } + versions := `"versions":{` + version("1.0.0") + `,` + version("2.0.0") + `,` + version("3.0.0") + `}` + times := `"time":{"created":"` + ts(1000*time.Hour) + `","1.0.0":"` + ts(900*time.Hour) + `","2.0.0":"` + ts(800*time.Hour) + `","3.0.0":"` + ts(time.Hour) + `"}` + tags := `"dist-tags":{"latest":"3.0.0","beta":"2.0.0","next":"1.0.0"}` + + docs := map[string]string{ + "testdata": string(packument), + "tags, versions, time": `{"name":"demo",` + tags + `,` + versions + `,` + times + `}`, + "time first": `{` + times + `,"name":"demo",` + versions + `,` + tags + `}`, + "no time": `{"name":"demo",` + tags + `,` + versions + `}`, + "time null": `{"time":null,` + tags + `,` + versions + `}`, + "tags not an object": `{"dist-tags":"x",` + versions + `,` + times + `}`, + "duplicates": `{"versions":{` + version("9.0.0") + `},` + tags + `,"time":{"9.0.0":"` + ts(time.Hour) + `"},` + + versions + `,` + times + `,"dist-tags":{"latest":"3.0.0"}}`, + "verbatim values": `{"_big":12345678901234567890,"readme":"&","nested":{"a":[1, 2 ,{"b":null}]},` + versions + `}`, + "unusual entries": `{"versions":{"1.0.0":"not an object","2.0.0":{"dist":"x"},"3.0.0":{"dist":{"tarball":7}},` + + `"4.0.0":{"dist":{"tarball":"https://registry.npmjs.org/demo/-/renamed.tgz"}},"5.0.0":{"dist":{}}}}`, + "whitespace": "{ \"name\" : \"demo\" ,\n\t\"versions\" : {\n \"1.0.0\" : { \"dist\" : { \"tarball\" : \"https://registry.npmjs.org/demo/-/demo-1.0.0.tgz\" } } } }", + "empty versions": `{"name":"demo","versions":{}}`, + } + options := map[string]Options{ + "plain": {ProxyURL: "http://proxy"}, + "cooldown": {ProxyURL: "http://proxy", Cooldown: &cooldown.Config{Default: "3d"}, EvaluatedAt: now}, + "denylist": {ProxyURL: "http://proxy", DeniedVersions: map[string]bool{"2.0.0": true}}, + "both": {ProxyURL: "http://proxy", Cooldown: &cooldown.Config{Default: "3d"}, EvaluatedAt: now, + DeniedVersions: map[string]bool{"1.0.0": true}}, + } + for docName, doc := range docs { + for optName, opts := range options { + t.Run(docName+"/"+optName, func(t *testing.T) { + want, wantKept, err := Rewrite("demo", []byte(doc), opts) + if err != nil { + t.Fatalf("Rewrite: %v", err) + } + got, gotKept, err := rewriteStream("demo", []byte(doc), opts) + if err != nil { + t.Fatalf("RewriteStream: %v", err) + } + // Rewrite keeps the whitespace inside the members of a + // filtered time map, which the stream doesn't see. + if !bytes.Equal(compact(t, got), compact(t, want)) { + t.Errorf("RewriteStream =\n%s\nRewrite =\n%s", got, want) + } + if docName == "verbatim values" && !bytes.Contains(got, []byte(`"_big":12345678901234567890,"readme":"&","nested":{"a":[1, 2 ,{"b":null}]}`)) { + t.Errorf("values not copied verbatim: %s", got) + } + slices.Sort(gotKept) + slices.Sort(wantKept) + if !slices.Equal(gotKept, wantKept) { + t.Errorf("kept %v, Rewrite kept %v", gotKept, wantKept) + } + }) + } + } +} + +func compact(t *testing.T, doc []byte) []byte { + t.Helper() + var out bytes.Buffer + if err := json.Compact(&out, doc); err != nil { + t.Fatalf("output is not JSON: %v\n%s", err, doc) + } + return out.Bytes() +} + +func TestRewriteStreamCallbacks(t *testing.T) { + var filtered []string + var rewritten []int + options := Options{ProxyURL: "http://proxy", Cooldown: &cooldown.Config{Default: "7d"}, + EvaluatedAt: time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), + OnCooldownFilter: func(version, _ string) { filtered = append(filtered, version) }, + OnTarballsRewritten: func(count int) { rewritten = append(rewritten, count) }, + } + if _, _, err := rewriteStream("@example/widgets", packument, options); err != nil { + t.Fatal(err) + } + if !slices.Equal(filtered, []string{"3.0.0"}) || !slices.Equal(rewritten, []int{2}) { + t.Errorf("filtered %v, rewritten %v; want [3.0.0] and [2]", filtered, rewritten) + } +} + +func TestRewriteStreamNotStreamable(t *testing.T) { + for name, tc := range map[string]struct { + doc string + options Options + }{ + "single version": {`{"name":"demo","version":"1.0.0","dist":{"tarball":"t"}}`, Options{}}, + "versions array": {`{"name":"demo","versions":[{"a":1}]}`, Options{}}, + "versions null": {`{"name":"demo","versions":null}`, Options{}}, + "pushed versions": {`{"versions":{}}`, Options{LocalVersions: []LocalVersion{{Version: "1.0.0", Entry: []byte(`{}`)}}}}, + } { + t.Run(name, func(t *testing.T) { + var out bytes.Buffer + scan, err := ScanPackument(strings.NewReader(tc.doc)) + if err != nil { + t.Fatal(err) + } + if _, err := RewriteStream("demo", scan, strings.NewReader(tc.doc), &out, tc.options); !errors.Is(err, ErrNotStreamable) { + t.Errorf("err = %v, want ErrNotStreamable", err) + } + if out.Len() != 0 { + t.Errorf("wrote %q before refusing", out.String()) + } + }) + } +} + +func TestScanPackument(t *testing.T) { + for _, body := range []string{`{"versions": {}`, `[1]`, `{"versions": {}} trailing`, `{"a":}`, ``} { + if _, err := ScanPackument(strings.NewReader(body)); err == nil { + t.Errorf("ScanPackument(%q): expected an error", body) + } + } + for body, want := range map[string]bool{ + `{"name":"demo","modified":"2024-01-01T00:00:00Z","versions":{}}`: true, + `{"name":"demo","modified":"2024-01-01T00:00:00Z","time":{},"versions":{}}`: false, + `{"name":"demo","versions":{}}`: false, + } { + scan, err := ScanPackument(strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + if scan.Abbreviated() != want || Abbreviated([]byte(body)) != want { + t.Errorf("Abbreviated(%s) = %v, want %v", body, scan.Abbreviated(), want) + } + } +} From 8c9e87f15ff8bc5c5022c5770176baf701e2d14d Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Fri, 30 Oct 2026 15:24:18 +0100 Subject: [PATCH 3/8] Stage fetched packuments, fall back on storage errors, stream version lookups A fetched packument is now stored under a staging path and checked before it replaces the cached copy, so an abbreviated one no longer overwrites a full copy; the full copy is served as when upstream fails. A failure to store falls back to the in-memory path, as caching is best effort there. Requests for a single version or dist-tag and the tarball download's cooldown and integrity lookup read the stored packument as a stream too, keeping only the one version entry they need. --- internal/handler/metadata_stored.go | 81 ++++++++-- internal/handler/npm.go | 223 +++++++++++++++++++++++---- internal/handler/npm_stream_test.go | 119 +++++++++++++- internal/handler/npm_test.go | 4 +- internal/metadata/npm/stream.go | 192 ++++++++++++++++++++--- internal/metadata/npm/stream_test.go | 73 ++++++++- 6 files changed, 611 insertions(+), 81 deletions(-) diff --git a/internal/handler/metadata_stored.go b/internal/handler/metadata_stored.go index e46239f0..d8313037 100644 --- a/internal/handler/metadata_stored.go +++ b/internal/handler/metadata_stored.go @@ -2,6 +2,7 @@ package handler import ( "context" + "crypto/rand" "crypto/sha256" "encoding/hex" "errors" @@ -36,8 +37,14 @@ var errStoredMetadataChanged = errors.New("stored metadata changed while it was // Concurrent calls for one document share a fetch, and instances sharing the // database take turns as for other metadata. // -// It returns errMetadataNotStored when the proxy doesn't cache metadata. -func (p *Proxy) storedMetadataDocument(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string) (*database.MetadataCacheEntry, error) { +// validate, when not nil, checks a fetched document before it replaces the +// stored one; a document it rejects is not stored, and the stored copy is +// served as when upstream fails. It does not check a stored copy within its +// TTL. +// +// It returns errMetadataNotStored when the proxy doesn't cache metadata or +// storing the document failed, and the caller reads it into memory instead. +func (p *Proxy) storedMetadataDocument(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, validate func(io.Reader) error) (*database.MetadataCacheEntry, error) { if !p.CacheMetadata || p.DB == nil || p.Storage == nil || p.MetadataTTL <= 0 { return nil, errMetadataNotStored } @@ -78,7 +85,7 @@ func (p *Proxy) storedMetadataDocument(ctx context.Context, ecosystem, cacheKey, } res := metadataResult{err: errSharedFetchAbandoned} defer func() { release(res.err != nil || res.stale) }() - res = p.fetchStoredMetadata(fetchCtx, ecosystem, cacheKey, upstreamURL, accept, entry) + res = p.fetchStoredMetadata(fetchCtx, ecosystem, cacheKey, upstreamURL, accept, validate, entry) return res }) return res.entry, res.err @@ -101,20 +108,21 @@ func usableStoredMetadata(entry *database.MetadataCacheEntry) bool { // fetchStoredMetadata fetches a document from upstream into storage, // revalidating entry when there is one, and falls back to entry when -// upstream fails. -func (p *Proxy) fetchStoredMetadata(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, entry *database.MetadataCacheEntry) metadataResult { +// upstream fails or validate rejects what it sent. A failure to store is +// returned as it is, for the caller to read the document into memory. +func (p *Proxy) fetchStoredMetadata(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, validate func(io.Reader) error, entry *database.MetadataCacheEntry) metadataResult { revalidate := entry if !usableStoredMetadata(entry) { revalidate = nil } - stored, err := p.storeUpstreamMetadata(ctx, ecosystem, cacheKey, upstreamURL, accept, revalidate) + stored, err := p.storeUpstreamMetadata(ctx, ecosystem, cacheKey, upstreamURL, accept, validate, revalidate) if errors.Is(err, errStale304) { - stored, err = p.storeUpstreamMetadata(ctx, ecosystem, cacheKey, upstreamURL, accept, nil) + stored, err = p.storeUpstreamMetadata(ctx, ecosystem, cacheKey, upstreamURL, accept, validate, nil) } if err == nil { return metadataResult{entry: stored} } - if !usableStoredMetadata(entry) || errors.Is(err, ErrUpstreamNotFound) { + if !usableStoredMetadata(entry) || errors.Is(err, ErrUpstreamNotFound) || errors.Is(err, errMetadataNotStored) { return metadataResult{err: err} } return p.staleStoredMetadata(ecosystem, cacheKey, entry, err) @@ -136,7 +144,10 @@ func (p *Proxy) staleStoredMetadata(ecosystem, cacheKey string, entry *database. // returns its new cache row. On a 304 for entry it only moves the row's // fetched_at on; errStale304 means the row was replaced meanwhile and the // caller should fetch without validators. -func (p *Proxy) storeUpstreamMetadata(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, entry *database.MetadataCacheEntry) (*database.MetadataCacheEntry, error) { +// +// With validate, the document is staged under a path of its own and checked +// there first, so one validate rejects never replaces the stored copy. +func (p *Proxy) storeUpstreamMetadata(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, validate func(io.Reader) error, entry *database.MetadataCacheEntry) (*database.MetadataCacheEntry, error) { resp, err := p.requestUpstreamMetadata(ctx, upstreamURL, entry, accept, "") if err != nil { return nil, err @@ -166,8 +177,17 @@ func (p *Proxy) storeUpstreamMetadata(ctx context.Context, ecosystem, cacheKey, } // Go decompresses the body, since no Accept-Encoding was set, so it is // stored uncompressed. - if _, err := p.storeMetadataBlob(ctx, ecosystem, cacheKey, metadataStoragePath(ecosystem, cacheKey), resp.Body, meta); err != nil { - return nil, err + var body io.Reader = resp.Body + if validate != nil { + staged, err := p.stageMetadata(ctx, ecosystem, cacheKey, resp.Body, validate) + if err != nil { + return nil, err + } + defer func() { _ = staged.Close() }() + body = staged + } + if _, err := p.storeMetadataBlob(ctx, ecosystem, cacheKey, metadataStoragePath(ecosystem, cacheKey), body, meta); err != nil { + return nil, fmt.Errorf("%w: %w", errMetadataNotStored, err) } stored, err := p.DB.GetMetadataCache(ecosystem, cacheKey) if err != nil || !usableStoredMetadata(stored) { @@ -176,6 +196,45 @@ func (p *Proxy) storeUpstreamMetadata(ctx context.Context, ecosystem, cacheKey, return stored, nil } +// stageMetadata stores body under a staging path of its own, checks it with +// validate, and returns it opened for reading, to be stored in place of the +// cached copy. The staged copy is deleted when the reader is closed. A +// failure to store it is returned as errMetadataNotStored. +func (p *Proxy) stageMetadata(ctx context.Context, ecosystem, cacheKey string, body io.Reader, validate func(io.Reader) error) (io.ReadCloser, error) { + path := metadataStoragePath(ecosystem, cacheKey) + ".staged-" + rand.Text() + discard := func() { + if err := p.Storage.Delete(context.WithoutCancel(ctx), path); err != nil { + p.Logger.Warn("failed to delete staged metadata", "ecosystem", ecosystem, "key", cacheKey, "error", err) + } + } + if _, _, err := p.Storage.Store(ctx, path, body); err != nil { + discard() + return nil, fmt.Errorf("%w: staging: %w", errMetadataNotStored, err) + } + if err := readStored(func() (io.ReadCloser, error) { return p.Storage.Open(ctx, path) }, validate); err != nil { + discard() + return nil, err + } + staged, err := p.Storage.Open(ctx, path) + if err != nil { + discard() + return nil, fmt.Errorf("%w: reading staged copy: %w", errMetadataNotStored, err) + } + return &stagedReader{ReadCloser: staged, discard: discard}, nil +} + +// stagedReader deletes the staged copy it reads when closed. +type stagedReader struct { + io.ReadCloser + discard func() +} + +func (s *stagedReader) Close() error { + err := s.ReadCloser.Close() + s.discard() + return err +} + // openStoredMetadata opens the document entry describes. Reading it to the // end fails with errStoredMetadataChanged when its bytes don't have the // digest entry records. diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 99c12aea..baddd97f 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -2,6 +2,7 @@ package handler import ( "bytes" + "context" "errors" "fmt" "io" @@ -12,6 +13,7 @@ import ( "time" "github.com/git-pkgs/integrity" + "github.com/git-pkgs/proxy/internal/database" "github.com/git-pkgs/proxy/internal/jsonscan" "github.com/git-pkgs/proxy/internal/metadata/npm" "github.com/git-pkgs/proxy/internal/retention" @@ -329,19 +331,13 @@ func (h *NPMHandler) serveStreamedPackument(w http.ResponseWriter, r *http.Reque if h.proxy.rewrites == nil { return false } - entry, err := h.proxy.storedMetadataDocument(r.Context(), "npm", metadataCacheKey, upstreamURL, accept) + entry, err := h.storedPackument(r.Context(), upstreamURL, accept, metadataCacheKey) switch { case err == nil: - case errors.Is(err, errMetadataNotStored): + case errors.Is(err, errInMemory): return false - case errors.Is(err, ErrUpstreamNotFound): - JSONError(w, http.StatusNotFound, ErrPackageNotFound.Error()) - return true - case writeUpstreamHeld(w, err): - return true default: - h.proxy.Logger.Error("failed to fetch npm metadata", "error", err) - JSONError(w, http.StatusBadGateway, "failed to fetch from upstream") + h.writeFetchError(w, err) return true } @@ -378,6 +374,109 @@ func (h *NPMHandler) serveStreamedPackument(w http.ResponseWriter, r *http.Reque return true } +// npmFetchError is a failure to fetch a packument, answered as such rather +// than as a failure to filter it. +type npmFetchError struct{ err error } + +func (e *npmFetchError) Error() string { return e.err.Error() } +func (e *npmFetchError) Unwrap() error { return e.err } + +// writeFetchError answers a request whose packument could not be fetched. +func (h *NPMHandler) writeFetchError(w http.ResponseWriter, err error) { + switch { + case errors.Is(err, ErrUpstreamNotFound): + JSONError(w, http.StatusNotFound, ErrPackageNotFound.Error()) + case writeUpstreamHeld(w, err): + default: + h.proxy.Logger.Error("failed to fetch npm metadata", "error", err) + JSONError(w, http.StatusBadGateway, "failed to fetch from upstream") + } +} + +// errInMemory is returned when a packument has to be read into memory to +// answer the request: metadata is not cached in storage, or the stored +// document can't be read as a stream. +var errInMemory = errors.New("npm packument needs the in-memory path") + +// storedPackument returns the cache row of the packument stored for cacheKey, +// fetching it into storage first when needed. It returns errInMemory when +// metadata isn't cached in storage, and ErrUpstreamNotFound and upstream +// errors as they come. +func (h *NPMHandler) storedPackument(ctx context.Context, packumentURL, accept, cacheKey string) (*database.MetadataCacheEntry, error) { + entry, err := h.proxy.storedMetadataDocument(ctx, "npm", cacheKey, packumentURL, accept, h.packumentStreamValidator()) + if errors.Is(err, errMetadataNotStored) { + return nil, errInMemory + } + return entry, err +} + +// packumentStreamValidator is packumentValidator for a packument read from +// storage. +func (h *NPMHandler) packumentStreamValidator() func(io.Reader) error { + if h.packumentValidator() == nil { + return nil + } + return func(r io.Reader) error { + entry, err := npm.LookupVersion(r, "") + if err != nil { + return err + } + if entry.Abbreviated { + return errAbbreviatedPackument + } + return nil + } +} + +// streamedVersionMetadata resolves a version or dist-tag through the +// packument stored in storage, as rewriteNPMVersionFromPackument does in +// memory: one pass collects what cooldown and the denylist decide on, and a +// second finds the version entry. It returns errInMemory when the request +// has to take the in-memory path. +func (h *NPMHandler) streamedVersionMetadata(ctx context.Context, packageName, specifier, packumentURL, accept, cacheKey, metadataCacheKey string) (rewrittenMetadata, error) { + if h.proxy.rewrites == nil { + return rewrittenMetadata{}, errInMemory + } + entry, err := h.storedPackument(ctx, packumentURL, accept, cacheKey) + if errors.Is(err, errInMemory) { + return rewrittenMetadata{}, err + } + if err != nil { + return rewrittenMetadata{}, &npmFetchError{err} + } + fullRequired := h.packumentValidator() != nil + options := h.npmRewriteOptions(packageName, nil) + m, err := h.proxy.streamedRewrite(ctx, "npm", h.proxyURL, metadataCacheKey, entry, func(open func() (io.ReadCloser, error), out io.Writer) error { + var scan *npm.Scan + err := readStored(open, func(in io.Reader) (err error) { + scan, err = npm.ScanPackument(in) + return err + }) + if err != nil { + return err + } + if fullRequired && scan.Abbreviated() { + return npm.ErrNotStreamable + } + return readStored(open, func(in io.Reader) error { + version, err := npm.VersionStream(packageName, specifier, scan, in, options) + if err != nil { + return npmRewriteError(err) + } + _, err = out.Write(version) + return err + }) + }) + switch { + case err == nil, ctx.Err() != nil, errors.Is(err, ErrVersionDenied), errors.Is(err, ErrVersionNotFound): + return m, err + case !errors.Is(err, npm.ErrNotStreamable): + h.proxy.Logger.Warn("failed to read stored npm metadata, reading it in memory", + "package", packageName, "error", err) + } + return rewrittenMetadata{}, errInMemory +} + // readStored opens a stored document and reads it with read. func readStored(open func() (io.ReadCloser, error), read func(io.Reader) error) error { in, err := open() @@ -442,7 +541,19 @@ func requireFullPackument(body []byte) error { // filtered packument so cooldown and dist-tags match the root metadata response. func (h *NPMHandler) handleFilteredNPMVersionMetadata(w http.ResponseWriter, r *http.Request, packageName, specifier, accept, registryURL, cacheKey, metadataCacheKey string) { _, packumentPath := npmMetadataPath(packageName) - packument, _, _, err := h.proxy.fetchOrCacheMetadata(r.Context(), "npm", cacheKey, registryURL+"/"+packumentPath, "", h.packumentValidator(), accept) + packumentURL := registryURL + "/" + packumentPath + rewritten, err := h.streamedVersionMetadata(r.Context(), packageName, specifier, packumentURL, accept, cacheKey, metadataCacheKey) + var fetchErr *npmFetchError + switch { + case errors.As(err, &fetchErr): + h.writeFetchError(w, fetchErr.err) + return + case !errors.Is(err, errInMemory): + h.writeVersionMetadata(w, r, packageName, rewritten, err) + return + } + + packument, _, _, err := h.proxy.fetchOrCacheMetadata(r.Context(), "npm", cacheKey, packumentURL, "", h.packumentValidator(), accept) if err != nil { if errors.Is(err, ErrUpstreamNotFound) { JSONError(w, http.StatusNotFound, ErrPackageNotFound.Error()) @@ -456,10 +567,16 @@ func (h *NPMHandler) handleFilteredNPMVersionMetadata(w http.ResponseWriter, r * return } - rewritten, err := h.proxy.cachedRewrite(r.Context(), "npm", h.proxyURL, metadataCacheKey, packument, "", func(b []byte) ([]byte, []string, error) { + rewritten, err = h.proxy.cachedRewrite(r.Context(), "npm", h.proxyURL, metadataCacheKey, packument, "", func(b []byte) ([]byte, []string, error) { out, err := h.rewriteNPMVersionFromPackument(packageName, specifier, b) return out, nil, err }) + h.writeVersionMetadata(w, r, packageName, rewritten, err) +} + +// writeVersionMetadata answers a version metadata request with its rewrite +// or the error that kept it from being made. +func (h *NPMHandler) writeVersionMetadata(w http.ResponseWriter, r *http.Request, packageName string, rewritten rewrittenMetadata, err error) { if err != nil { if r.Context().Err() != nil { return @@ -620,10 +737,8 @@ func (h *NPMHandler) handleDownload(w http.ResponseWriter, r *http.Request) { return } registryURL, cacheKey := h.sourceFor(packageName) - metadata := sync.OnceValues(func() ([]byte, error) { - upstreamURL := registryURL + "/" + url.PathEscape(packageName) - body, _, _, err := h.proxy.fetchOrCacheMetadata(r.Context(), "npm", cacheKey, upstreamURL, "", h.packumentValidator(), h.packumentAccept()) - return body, err + metadata := sync.OnceValues(func() (npmVersionData, error) { + return h.versionData(r.Context(), registryURL+"/"+url.PathEscape(packageName), cacheKey, version) }) inCooldown, err := h.versionInCooldown(packageName, version, metadata) if err != nil { @@ -665,7 +780,7 @@ func (h *NPMHandler) handleDownload(w http.ResponseWriter, r *http.Request) { ServeArtifactRequest(w, r, result) } -func (h *NPMHandler) getTarball(r *http.Request, registryURL, packageName, version, filename string, metadata func() ([]byte, error)) (*CacheResult, error) { +func (h *NPMHandler) getTarball(r *http.Request, registryURL, packageName, version, filename string, metadata func() (npmVersionData, error)) (*CacheResult, error) { cacheFilename := h.routes.cacheFilename(packageName, filename) if cached, err := h.proxy.GetCachedArtifact(r.Context(), "npm", packageName, version, cacheFilename); err != nil || cached != nil { return cached, err @@ -673,35 +788,35 @@ func (h *NPMHandler) getTarball(r *http.Request, registryURL, packageName, versi // The packument says where the tarball is and what it must hash to. // Without it the tarball could not be verified, and an unverified copy // would be cached and served to everyone, so the download fails instead. - body, err := metadata() + data, err := metadata() if err != nil { return nil, fmt.Errorf("fetching npm metadata to verify tarball: %w", err) } downloadURL := fmt.Sprintf("%s/%s/-/%s", registryURL, escapeNPMDownloadPackage(packageName), url.PathEscape(filename)) - if tarball := npmVersionTarball(body, version); tarball != "" { + if tarball := npmEntryTarball(data.entry); tarball != "" { downloadURL, err = validateTarballURL(tarball, registryURL) if err != nil { return nil, err } } - sri := npmVersionIntegrity(body, version) + sri := npmEntryIntegrity(data.entry) return h.proxy.GetOrFetchArtifactFromURLWithIntegrity(r.Context(), "npm", packageName, version, cacheFilename, downloadURL, sri) } -// npmVersionIntegrity returns the integrity a packument declares for a -// version's tarball: dist.integrity when it is a Subresource Integrity string +// npmEntryIntegrity returns the integrity a version's packument entry +// declares for its tarball: dist.integrity when it is a Subresource Integrity string // the proxy can verify, or else the SHA-1 in dist.shasum, which is all // versions published before integrity existed carry, as "sha1-". It // returns "" when neither is usable, and the tarball is fetched unverified as // before. -func npmVersionIntegrity(body []byte, version string) string { - sri, _, _ := jsonscan.LookupString(body, "versions", version, "dist", "integrity") +func npmEntryIntegrity(entry []byte) string { + sri, _, _ := jsonscan.LookupString(entry, "dist", "integrity") if sri != "" { if _, err := integrity.ParseSRI(sri); err == nil { return sri } } - shasum, _, _ := jsonscan.LookupString(body, "versions", version, "dist", "shasum") + shasum, _, _ := jsonscan.LookupString(entry, "dist", "shasum") return sha1Checksum(shasum) } @@ -709,10 +824,57 @@ func npmVersionIntegrity(body []byte, version string) string { // or "" if it has none. It reads that one value in place rather than decoding // every version, since it runs on each tarball the proxy has not cached yet. func npmVersionTarball(body []byte, version string) string { - tarball, _, _ := jsonscan.LookupString(body, "versions", version, "dist", "tarball") + entry, _ := jsonscan.Lookup(body, "versions", version) + return npmEntryTarball(entry) +} + +// npmEntryTarball is npmVersionTarball for the version's entry. +func npmEntryTarball(entry []byte) string { + tarball, _, _ := jsonscan.LookupString(entry, "dist", "tarball") return tarball } +// npmVersionData is what a tarball download reads from the packument: the +// version's entry, nil when it has none, and its publish time from the time +// map, "" when it has none. +type npmVersionData struct { + entry []byte + published string +} + +// versionData reads what a tarball download needs about version from the +// packument: from storage in one pass when metadata is cached there, and +// otherwise from the packument read into memory. +func (h *NPMHandler) versionData(ctx context.Context, packumentURL, cacheKey, version string) (npmVersionData, error) { + accept := h.packumentAccept() + if entry, err := h.storedPackument(ctx, packumentURL, accept, cacheKey); err == nil { + var found npm.VersionEntry + err := readStored(func() (io.ReadCloser, error) { return h.proxy.openStoredMetadata(ctx, entry) }, func(in io.Reader) (err error) { + found, err = npm.LookupVersion(in, version) + return err + }) + switch { + case err == nil && (!found.Abbreviated || h.packumentValidator() == nil): + return npmVersionData{entry: found.Entry, published: found.Published}, nil + case err != nil: + h.proxy.Logger.Warn("failed to read stored npm metadata, reading it in memory", "url", packumentURL, "error", err) + } + } else if !errors.Is(err, errInMemory) { + return npmVersionData{}, err + } + + body, _, _, err := h.proxy.fetchOrCacheMetadata(ctx, "npm", cacheKey, packumentURL, "", h.packumentValidator(), accept) + if err != nil { + return npmVersionData{}, err + } + entry, _ := jsonscan.Lookup(body, "versions", version) + published, _, err := jsonscan.LookupString(body, "time", version) + if err != nil { + return npmVersionData{}, fmt.Errorf("parsing npm metadata: %w", err) + } + return npmVersionData{entry: entry, published: published}, nil +} + func validateTarballURL(raw, registryURL string) (string, error) { tarball, err := url.Parse(raw) if err != nil { @@ -756,7 +918,7 @@ func validateTarballURL(raw, registryURL string) (string, error) { // for a private version of the same name, a private time would hold back the // public version, and moving a route to another upstream would keep the old // one's times. -func (h *NPMHandler) versionInCooldown(packageName, version string, metadata func() ([]byte, error)) (bool, error) { +func (h *NPMHandler) versionInCooldown(packageName, version string, metadata func() (npmVersionData, error)) (bool, error) { if !h.proxy.cooldownApplies("npm", packageName) { return false, nil } @@ -769,7 +931,7 @@ func (h *NPMHandler) versionInCooldown(packageName, version string, metadata fun } } - body, err := metadata() + data, err := metadata() if errors.Is(err, ErrUpstreamNotFound) { return false, nil } @@ -777,11 +939,8 @@ func (h *NPMHandler) versionInCooldown(packageName, version string, metadata fun return false, err } - published, ok, err := jsonscan.LookupString(body, "time", version) - if err != nil { - return false, fmt.Errorf("parsing npm metadata: %w", err) - } - if !ok { + published := data.published + if published == "" { return false, nil } diff --git a/internal/handler/npm_stream_test.go b/internal/handler/npm_stream_test.go index 565f0627..3442ce2c 100644 --- a/internal/handler/npm_stream_test.go +++ b/internal/handler/npm_stream_test.go @@ -16,12 +16,18 @@ import ( "time" "github.com/git-pkgs/cooldown" + "github.com/git-pkgs/registries/fetch" ) // bigPackument returns a packument laid out as registry.npmjs.org writes // them, dist-tags before versions and time after, with n old versions padded // so the document is large, and a latest published an hour ago. func bigPackument(n int) string { + return bigPackumentAt("https://registry.npmjs.org", n) +} + +// bigPackumentAt is bigPackument with tarballs under base. +func bigPackumentAt(base string, n int) string { var b strings.Builder b.WriteString(`{"_id":"big","name":"big","dist-tags":{"latest":"9.0.0","old":"1.0.0"},"versions":{`) times := []string{`"created":"2015-01-01T00:00:00Z"`} @@ -30,12 +36,12 @@ func bigPackument(n int) string { if i == 0 { v = "1.0.0" } - fmt.Fprintf(&b, `%q:{"name":"big","version":%q,"readme":%q,"dist":{"tarball":"https://registry.npmjs.org/big/-/big-%s.tgz"}},`, - v, v, strings.Repeat("x", 512), v) + fmt.Fprintf(&b, `%q:{"name":"big","version":%q,"readme":%q,"dist":{"tarball":"%s/big/-/big-%s.tgz"}},`, + v, v, strings.Repeat("x", 512), base, v) times = append(times, fmt.Sprintf(`%q:"2016-01-01T00:%02d:%02dZ"`, v, i/60%60, i%60)) } recent := time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) - b.WriteString(`"9.0.0":{"name":"big","version":"9.0.0","dist":{"tarball":"https://registry.npmjs.org/big/-/big-9.0.0.tgz"}}},`) + b.WriteString(`"9.0.0":{"name":"big","version":"9.0.0","dist":{"tarball":"` + base + `/big/-/big-9.0.0.tgz"}}},`) times = append(times, `"9.0.0":"`+recent+`"`) b.WriteString(`"time":{` + strings.Join(times, ",") + `},"readme":"big"}`) return b.String() @@ -200,7 +206,7 @@ func TestNPMStreamedPackumentStale(t *testing.T) { func TestNPMStreamedPackumentChangedInStorage(t *testing.T) { u := newStreamUpstream(t, bigPackument(10)) proxy, store, _ := streamProxy(t, u) - entry, err := proxy.storedMetadataDocument(context.Background(), "npm", "big", u.URL+"/big", contentTypeJSON) + entry, err := proxy.storedMetadataDocument(context.Background(), "npm", "big", u.URL+"/big", contentTypeJSON, nil) if err != nil { t.Fatal(err) } @@ -225,6 +231,9 @@ func TestNPMStreamedPackumentRevalidates(t *testing.T) { proxy, store, handler := streamProxy(t, u) first := serve(handler, "/big", nil) decodeStreamed(t, first) + store.mu.Lock() + stores := store.stores + store.mu.Unlock() proxy.MetadataTTL = time.Nanosecond second := serve(handler, "/big", nil) @@ -237,7 +246,105 @@ func TestNPMStreamedPackumentRevalidates(t *testing.T) { } store.mu.Lock() defer store.mu.Unlock() - if store.stores != 1 { - t.Errorf("document stored %d times, want once", store.stores) + if store.stores != stores { + t.Errorf("the 304 stored the document again (%d stores, %d before)", store.stores, stores) + } +} + +// TestNPMStreamedPackumentKeepsFullCopy checks that an abbreviated packument +// from upstream, where cooldown needs the full one, doesn't replace the full +// copy in storage, which is served instead as when upstream fails. +func TestNPMStreamedPackumentKeepsFullCopy(t *testing.T) { + full := bigPackument(10) + u := newStreamUpstream(t, full) + proxy, store, handler := streamProxy(t, u) + decodeStreamed(t, serve(handler, "/big", nil)) + + proxy.MetadataTTL = time.Nanosecond + proxy.SetMetadataRewriteCacheSize(64 << 20) + u.body.Store(`{"name":"big","modified":"2026-01-01T00:00:00Z","dist-tags":{"latest":"1.0.0"},"versions":{"1.0.0":{}}}`) + if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != 10 { + t.Errorf("served %d versions, want the 10 of the full copy", len(doc.Versions)) + } + + store.mu.Lock() + defer store.mu.Unlock() + if got := string(store.files[metadataStoragePath("npm", "big")]); got != full { + t.Errorf("stored packument was replaced: %.80s", got) + } + for path := range store.files { + if strings.Contains(path, ".staged-") { + t.Errorf("staged copy %s left in storage", path) + } + } +} + +// TestNPMStreamedPackumentStorageFails checks that a packument is still +// served, read into memory, when storing it fails. +func TestNPMStreamedPackumentStorageFails(t *testing.T) { + u := newStreamUpstream(t, bigPackument(10)) + _, store, handler := streamProxy(t, u) + store.storeErr = errors.New("no space left on device") + doc := decodeStreamed(t, serve(handler, "/big", nil)) + if len(doc.Versions) != 10 || doc.DistTags["latest"] != "1.0.9" { + t.Errorf("served %d versions, latest %q; want 10 and 1.0.9", len(doc.Versions), doc.DistTags["latest"]) + } +} + +// TestNPMStreamedVersionMetadata resolves versions and dist-tags of a +// packument larger than metadata_max_size. +func TestNPMStreamedVersionMetadata(t *testing.T) { + body := bigPackument(100) + u := newStreamUpstream(t, body) + proxy, _, handler := streamProxy(t, u) + proxy.MetadataMaxSize = int64(len(body) / 4) + + for path, want := range map[string]string{ + "/big/1.0.5": "1.0.5", + "/big/latest": "1.0.99", // 9.0.0 is inside cooldown + "/big/old": "1.0.0", + } { + rec := serve(handler, path, nil) + var v struct { + Version string `json:"version"` + Dist struct { + Tarball string `json:"tarball"` + } `json:"dist"` + } + if rec.Code != http.StatusOK || json.Unmarshal(rec.Body.Bytes(), &v) != nil { + t.Errorf("%s: status %d: %.200s", path, rec.Code, rec.Body.String()) + continue + } + if v.Version != want || v.Dist.Tarball != "http://proxy.example/npm/big/-/big-"+want+".tgz" { + t.Errorf("%s = %s with tarball %s, want %s", path, v.Version, v.Dist.Tarball, want) + } + } + if rec := serve(handler, "/big/9.0.0", nil); rec.Code != http.StatusNotFound { + t.Errorf("version inside cooldown: status %d, want 404", rec.Code) + } + if n := u.requests.Load(); n != 1 { + t.Errorf("upstream asked %d times, want once", n) + } +} + +// TestNPMStreamedTarballCooldown checks the tarball download against a +// packument larger than metadata_max_size: the version's publish time and +// tarball URL are read from storage. +func TestNPMStreamedTarballCooldown(t *testing.T) { + u := newStreamUpstream(t, "") + u.body.Store(bigPackumentAt(u.URL, 100)) + proxy, _, handler := streamProxy(t, u) + proxy.MetadataMaxSize = 1024 + fetcher := proxy.Fetcher.(*mockFetcher) + fetcher.artifact = &fetch.Artifact{Body: io.NopCloser(strings.NewReader("package"))} + + if rec := serve(handler, "/big/-/big-9.0.0.tgz", nil); rec.Code != http.StatusNotFound { + t.Errorf("tarball inside cooldown: status %d, want 404", rec.Code) + } + if rec := serve(handler, "/big/-/big-1.0.5.tgz", nil); rec.Code != http.StatusOK || rec.Body.String() != "package" { + t.Errorf("tarball: status %d, body %q", rec.Code, rec.Body.String()) + } + if want := u.URL + "/big/-/big-1.0.5.tgz"; fetcher.fetchedURL != want { + t.Errorf("fetched %s, want %s from the packument", fetcher.fetchedURL, want) } } diff --git a/internal/handler/npm_test.go b/internal/handler/npm_test.go index 20fda4e4..9b9a6c35 100644 --- a/internal/handler/npm_test.go +++ b/internal/handler/npm_test.go @@ -561,9 +561,9 @@ func TestNPMDownloadCooldownDisabled(t *testing.T) { proxy, _, _, _ := setupTestProxy(t) h := NewNPMHandler(proxy, "http://proxy.test", "") - if inCooldown, _ := h.versionInCooldown("leftpad", testVersion100, func() ([]byte, error) { + if inCooldown, _ := h.versionInCooldown("leftpad", testVersion100, func() (npmVersionData, error) { t.Fatal("cooldown must not request metadata when disabled") - return nil, nil + return npmVersionData{}, nil }); inCooldown { t.Error("versionInCooldown = true, want false when cooldown is not configured") } diff --git a/internal/metadata/npm/stream.go b/internal/metadata/npm/stream.go index cc70c52b..3e86faa5 100644 --- a/internal/metadata/npm/stream.go +++ b/internal/metadata/npm/stream.go @@ -99,50 +99,63 @@ func RewriteStream(packageName string, scan *Scan, r io.Reader, w io.Writer, opt return nil, ErrNotStreamable } h := &transformer{options} + f := scan.filter(h, packageName) + sw := &streamWriter{ + transformer: h, + packageName: packageName, + scan: scan, + filtered: f, + seen: map[string]int{}, + } + if err := jsonstream.CopyObject(json.NewDecoder(r), w, sw.member); err != nil { + return nil, err + } + h.tarballsRewritten(sw.rewritten) + return slices.Collect(maps.Keys(f.versions)), nil +} + +// filtered is what cooldown and the denylist leave of a scanned packument. +type filtered struct { + versions map[string]any + timeMap, distTags map[string]any + timeChanged, tagsChanged bool +} - versions := make(map[string]any, len(scan.versions)) - for _, v := range scan.versions { +// filter runs the filters Rewrite runs over what scan collected. The scan is +// left as it is, so it can be filtered again. +func (s *Scan) filter(h *transformer, packageName string) filtered { + versions := make(map[string]any, len(s.versions)) + for _, v := range s.versions { versions[v] = nil } metadata := map[string]any{} - timeMap := scan.objectValue(metadata, "time", scan.time) - distTags := scan.objectValue(metadata, "dist-tags", scan.distTags) + timeMap := s.objectValue(metadata, "time", s.time) + distTags := s.objectValue(metadata, "dist-tags", s.distTags) timeEntries := len(timeMap) originalTags := maps.Clone(distTags) h.applyCooldownFiltering(metadata, versions, packageName) h.applyDenylistFiltering(metadata, versions) - sw := &streamWriter{ - transformer: h, - packageName: packageName, - scan: scan, + return filtered{ versions: versions, timeMap: timeMap, distTags: distTags, timeChanged: timeMap != nil && len(timeMap) != timeEntries, tagsChanged: distTags != nil && !reflect.DeepEqual(distTags, originalTags), - seen: map[string]int{}, - } - if err := jsonstream.CopyObject(json.NewDecoder(r), w, sw.member); err != nil { - return nil, err } - h.tarballsRewritten(sw.rewritten) - return slices.Collect(maps.Keys(versions)), nil } // streamWriter writes the second pass of RewriteStream from the filtered // state the first pass left. type streamWriter struct { *transformer - packageName string - scan *Scan - versions map[string]any - timeMap, distTags map[string]any - timeChanged, tagsChanged bool - seen map[string]int - entry bytes.Buffer - rewritten int + filtered + packageName string + scan *Scan + seen map[string]int + entry bytes.Buffer + rewritten int } // member writes one top-level member. Members the filters don't touch are @@ -201,13 +214,19 @@ func (s *streamWriter) time(t *jsonstream.Member) error { } // objectValue records value under name in metadata, as Rewrite's -// decodeObject does, and returns it when it is an object. +// decodeObject does, and returns it when it is an object. An object is +// copied, since the filters edit it. func (s *Scan) objectValue(metadata map[string]any, name string, value any) map[string]any { if s.occurrences[name] == 0 { return nil } - metadata[name] = value - object, _ := value.(map[string]any) + object, ok := value.(map[string]any) + if !ok { + metadata[name] = value + return nil + } + object = maps.Clone(object) + metadata[name] = object return object } @@ -268,3 +287,126 @@ func skipValue(dec *json.Decoder) error { var raw json.RawMessage return dec.Decode(&raw) } + +// VersionStream is Version for a packument too large to hold in memory. scan +// is the result of ScanPackument over the bytes r reads again; the second +// pass only looks for the one version entry it serves. For a document +// without a versions object it returns ErrNotStreamable. +func VersionStream(packageName, specifier string, scan *Scan, r io.Reader, options Options) ([]byte, error) { + if !scan.versionsObject || len(options.LocalVersions) > 0 { + return nil, ErrNotStreamable + } + h := &transformer{options} + f := scan.filter(h, packageName) + + target := specifier + if tag, ok := f.distTags[specifier].(string); ok { + target = tag + } + if options.DeniedVersions[target] { + return nil, ErrVersionDenied + } + if _, kept := f.versions[target]; !kept { + return nil, ErrVersionNotFound + } + entry, err := LookupVersion(r, target) + if err != nil { + return nil, err + } + if entry.Entry == nil { + return nil, ErrVersionNotFound + } + var out bytes.Buffer + h.writeNPMVersion(&out, packageName, target, entry.Entry) + return out.Bytes(), nil +} + +// VersionEntry is what a packument holds for one version. +type VersionEntry struct { + // Entry is versions[version], or nil when there is none. + Entry json.RawMessage + // Published is time[version], or "" when it is missing or not a string. + Published string + // Abbreviated is set for an abbreviated packument (see Abbreviated). + Abbreviated bool +} + +// LookupVersion reads a packument once and returns what it holds for +// version, keeping no other version's entry. Of repeated keys the last one +// counts, as with JSON.parse. +func LookupVersion(r io.Reader, version string) (VersionEntry, error) { + var e VersionEntry + var hasTime, hasModified bool + dec := json.NewDecoder(r) + if err := expectDelim(dec, '{'); err != nil { + return e, err + } + for dec.More() { + key, err := objectKey(dec) + if err != nil { + return e, err + } + switch key { + case "versions": + e.Entry, err = objectMember(dec, version) + case "time": + hasTime = true + var raw json.RawMessage + raw, err = objectMember(dec, version) + e.Published = "" + if err == nil && raw != nil { + _ = json.Unmarshal(raw, &e.Published) + } + default: + hasModified = hasModified || key == "modified" + err = skipValue(dec) + } + if err != nil { + return e, err + } + } + if err := expectDelim(dec, '}'); err != nil { + return e, err + } + if _, err := dec.Token(); err != io.EOF { + if err == nil { + err = errors.New("unexpected data after the top-level object") + } + return e, err + } + e.Abbreviated = hasModified && !hasTime + return e, nil +} + +// objectMember reads a value and returns its member key when it is an object +// that has one, the last if repeated, or nil. +func objectMember(dec *json.Decoder, key string) (json.RawMessage, error) { + tok, err := dec.Token() + if err != nil { + return nil, err + } + delim, ok := tok.(json.Delim) + if !ok { + return nil, nil + } + var found json.RawMessage + for dec.More() { + var k string + if delim == '{' { + if k, err = objectKey(dec); err != nil { + return nil, err + } + } + var raw json.RawMessage + if err := dec.Decode(&raw); err != nil { + return nil, err + } + if delim == '{' && k == key { + found = raw + } + } + if _, err := dec.Token(); err != nil { + return nil, err + } + return found, nil +} diff --git a/internal/metadata/npm/stream_test.go b/internal/metadata/npm/stream_test.go index ef2c9542..0c8a4cee 100644 --- a/internal/metadata/npm/stream_test.go +++ b/internal/metadata/npm/stream_test.go @@ -24,11 +24,10 @@ func rewriteStream(packageName string, body []byte, options Options) ([]byte, [] return out.Bytes(), kept, err } -// TestRewriteStreamMatchesRewrite checks that the streamed rewrite writes the -// same bytes and keeps the same versions as Rewrite, over documents with the -// filtered members in either order, duplicates, and values Rewrite copies -// verbatim. -func TestRewriteStreamMatchesRewrite(t *testing.T) { +// streamCases returns packuments with the filtered members in either +// order, duplicates and values Rewrite copies verbatim, and the options to +// rewrite them with. +func streamCases() (map[string]string, map[string]Options) { now := time.Date(2026, 10, 18, 12, 0, 0, 0, time.UTC) ts := func(age time.Duration) string { return now.Add(-age).Format(time.RFC3339) } version := func(v string) string { @@ -60,6 +59,15 @@ func TestRewriteStreamMatchesRewrite(t *testing.T) { "both": {ProxyURL: "http://proxy", Cooldown: &cooldown.Config{Default: "3d"}, EvaluatedAt: now, DeniedVersions: map[string]bool{"1.0.0": true}}, } + return docs, options +} + +// TestRewriteStreamMatchesRewrite checks that the streamed rewrite writes the +// same bytes and keeps the same versions as Rewrite, over documents with the +// filtered members in either order, duplicates, and values Rewrite copies +// verbatim. +func TestRewriteStreamMatchesRewrite(t *testing.T) { + docs, options := streamCases() for docName, doc := range docs { for optName, opts := range options { t.Run(docName+"/"+optName, func(t *testing.T) { @@ -160,3 +168,58 @@ func TestScanPackument(t *testing.T) { } } } + +// TestVersionStreamMatchesVersion checks that a single version resolved from +// the stream is the one Version serves, with the same errors. +func TestVersionStreamMatchesVersion(t *testing.T) { + docs, options := streamCases() + specifiers := []string{"latest", "beta", "next", "old", "1.0.0", "2.0.0", "3.0.0", "9.0.0", "missing"} + for docName, doc := range docs { + scan, err := ScanPackument(strings.NewReader(doc)) + if err != nil { + t.Fatalf("%s: %v", docName, err) + } + for optName, opts := range options { + for _, spec := range specifiers { + want, wantErr := Version("demo", spec, []byte(doc), opts) + got, gotErr := VersionStream("demo", spec, scan, strings.NewReader(doc), opts) + name := docName + "/" + optName + "/" + spec + switch { + case wantErr != nil || gotErr != nil: + if !errors.Is(gotErr, ErrVersionDenied) == errors.Is(wantErr, ErrVersionDenied) || + !errors.Is(gotErr, ErrVersionNotFound) == errors.Is(wantErr, ErrVersionNotFound) || + (gotErr == nil) != (wantErr == nil) { + t.Errorf("%s: error %v, Version gave %v", name, gotErr, wantErr) + } + case !bytes.Equal(got, want): + t.Errorf("%s:\n%s\nVersion:\n%s", name, got, want) + } + } + } + } +} + +func TestLookupVersion(t *testing.T) { + doc := `{"modified":"x","versions":{"1.0.0":{"a":1}},"time":{"1.0.0":"2020-01-01T00:00:00Z","2.0.0":7},` + + `"versions":{"1.0.0":{"a":2},"2.0.0":{"b":1}}}` + for version, want := range map[string]VersionEntry{ + "1.0.0": {Entry: json.RawMessage(`{"a":2}`), Published: "2020-01-01T00:00:00Z"}, + "2.0.0": {Entry: json.RawMessage(`{"b":1}`)}, + "3.0.0": {}, + } { + got, err := LookupVersion(strings.NewReader(doc), version) + if err != nil { + t.Fatal(err) + } + if string(got.Entry) != string(want.Entry) || got.Published != want.Published || got.Abbreviated { + t.Errorf("LookupVersion(%s) = %+v, want %+v", version, got, want) + } + } + got, err := LookupVersion(strings.NewReader(`{"modified":"x","versions":{}}`), "1.0.0") + if err != nil || !got.Abbreviated { + t.Errorf("abbreviated packument: %+v, %v", got, err) + } + if _, err := LookupVersion(strings.NewReader(`{"versions":{}`), "1.0.0"); err == nil { + t.Error("expected an error for truncated JSON") + } +} From 760623e617d7bed7d3b64b437fb942076eef4504 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Fri, 30 Oct 2026 19:25:09 +0100 Subject: [PATCH 4/8] Serve the stored npm packument on upstream 404 and refetch on a 304 for a missing blob --- internal/handler/metadata_stored.go | 11 +++-- internal/handler/npm_stream_test.go | 72 +++++++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/internal/handler/metadata_stored.go b/internal/handler/metadata_stored.go index d8313037..80cbaf46 100644 --- a/internal/handler/metadata_stored.go +++ b/internal/handler/metadata_stored.go @@ -122,7 +122,9 @@ func (p *Proxy) fetchStoredMetadata(ctx context.Context, ecosystem, cacheKey, up if err == nil { return metadataResult{entry: stored} } - if !usableStoredMetadata(entry) || errors.Is(err, ErrUpstreamNotFound) || errors.Is(err, errMetadataNotStored) { + // As in fetchMetadataFromUpstream, any upstream failure, a 404 included, + // falls back to the stored copy. + if !usableStoredMetadata(entry) || errors.Is(err, errMetadataNotStored) { return metadataResult{err: err} } return p.staleStoredMetadata(ecosystem, cacheKey, entry, err) @@ -142,8 +144,8 @@ func (p *Proxy) staleStoredMetadata(ecosystem, cacheKey string, entry *database. // storeUpstreamMetadata fetches a document and streams it into storage, and // returns its new cache row. On a 304 for entry it only moves the row's -// fetched_at on; errStale304 means the row was replaced meanwhile and the -// caller should fetch without validators. +// fetched_at on; errStale304 means the stored copy is gone or the row was +// replaced meanwhile, and the caller should fetch without validators. // // With validate, the document is staged under a path of its own and checked // there first, so one validate rejects never replaces the stored copy. @@ -155,6 +157,9 @@ func (p *Proxy) storeUpstreamMetadata(ctx context.Context, ecosystem, cacheKey, defer func() { _ = resp.Body.Close() }() if resp.StatusCode == http.StatusNotModified && entry != nil { + if ok, err := p.Storage.Exists(ctx, entry.StoragePath); err != nil || !ok { + return nil, errStale304 + } refreshed, err := p.DB.RefreshMetadataCache(ecosystem, cacheKey, entry.ContentDigest.String, time.Now()) if err != nil || !refreshed { return nil, errStale304 diff --git a/internal/handler/npm_stream_test.go b/internal/handler/npm_stream_test.go index 3442ce2c..de5a17d2 100644 --- a/internal/handler/npm_stream_test.go +++ b/internal/handler/npm_stream_test.go @@ -50,24 +50,38 @@ func bigPackumentAt(base string, n int) string { type streamUpstream struct { *httptest.Server body atomic.Value // string + etag atomic.Value // string fail atomic.Bool + gone atomic.Bool requests atomic.Int32 + // sent counts the responses that carried the document. + sent atomic.Int32 +} + +// replace makes upstream send body under a new ETag, so a revalidation +// gets it in a 200. +func (u *streamUpstream) replace(body, etag string) { + u.body.Store(body) + u.etag.Store(etag) } func newStreamUpstream(t *testing.T, body string) *streamUpstream { t.Helper() u := &streamUpstream{} - u.body.Store(body) + u.replace(body, `"v1"`) u.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { u.requests.Add(1) switch { case u.fail.Load(): http.Error(w, "down", http.StatusBadGateway) - case r.URL.Path == "/big" && r.Header.Get("If-None-Match") == `"v1"`: + case u.gone.Load(): + http.NotFound(w, r) + case r.URL.Path == "/big" && r.Header.Get("If-None-Match") == u.etag.Load().(string): w.WriteHeader(http.StatusNotModified) case r.URL.Path == "/big": + u.sent.Add(1) w.Header().Set("Content-Type", "application/json") - w.Header().Set("ETag", `"v1"`) + w.Header().Set("ETag", u.etag.Load().(string)) _, _ = io.WriteString(w, u.body.Load().(string)) default: http.NotFound(w, r) @@ -201,6 +215,53 @@ func TestNPMStreamedPackumentStale(t *testing.T) { } } +// TestNPMStreamedPackumentStaleOn404 checks that a stored copy past its TTL +// is served when upstream answers 404, as the in-memory path does. +func TestNPMStreamedPackumentStaleOn404(t *testing.T) { + u := newStreamUpstream(t, bigPackument(10)) + proxy, _, handler := streamProxy(t, u) + decodeStreamed(t, serve(handler, "/big", nil)) + + proxy.MetadataTTL = time.Nanosecond + u.gone.Store(true) + for _, size := range []int64{64 << 20, 0} { // streamed, then in memory + proxy.SetMetadataRewriteCacheSize(size) + if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != 10 { + t.Errorf("rewrite cache %d: stale copy has %d versions, want 10", size, len(doc.Versions)) + } + } +} + +// TestNPMStreamedPackumentBlobMissing checks that a 304 for a row whose +// document is gone from storage fetches the document again, also when it is +// larger than metadata_max_size. +func TestNPMStreamedPackumentBlobMissing(t *testing.T) { + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, store, handler := streamProxy(t, u) + proxy.MetadataMaxSize = int64(len(body) / 4) + decodeStreamed(t, serve(handler, "/big", nil)) + + path := metadataStoragePath("npm", "big") + store.mu.Lock() + delete(store.files, path) + store.mu.Unlock() + proxy.MetadataTTL = time.Nanosecond + proxy.SetMetadataRewriteCacheSize(64 << 20) + + if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != 400 { + t.Errorf("served %d versions, want 400", len(doc.Versions)) + } + if n := u.sent.Load(); n != 2 { + t.Errorf("upstream sent the document %d times, want 2", n) + } + store.mu.Lock() + defer store.mu.Unlock() + if string(store.files[path]) != body { + t.Error("the document was not stored again") + } +} + // TestNPMStreamedPackumentChangedInStorage checks that a document replaced // in storage between the two passes isn't rewritten from mixed bytes. func TestNPMStreamedPackumentChangedInStorage(t *testing.T) { @@ -262,10 +323,13 @@ func TestNPMStreamedPackumentKeepsFullCopy(t *testing.T) { proxy.MetadataTTL = time.Nanosecond proxy.SetMetadataRewriteCacheSize(64 << 20) - u.body.Store(`{"name":"big","modified":"2026-01-01T00:00:00Z","dist-tags":{"latest":"1.0.0"},"versions":{"1.0.0":{}}}`) + u.replace(`{"name":"big","modified":"2026-01-01T00:00:00Z","dist-tags":{"latest":"1.0.0"},"versions":{"1.0.0":{}}}`, `"v2"`) if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != 10 { t.Errorf("served %d versions, want the 10 of the full copy", len(doc.Versions)) } + if n := u.sent.Load(); n != 2 { + t.Fatalf("upstream sent a document %d times, want 2: the abbreviated one was not fetched", n) + } store.mu.Lock() defer store.mu.Unlock() From 11ff4cf314c1d7dbeaf45167d01b2559b6543472 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 31 Oct 2026 16:55:15 +0100 Subject: [PATCH 5/8] Fetch an npm packument again when its stored copy is gone within the TTL A row within its TTL was served without checking its blob, so a missing blob failed the streamed read and the request fell back to the bounded in-memory path. A read that finds the stored copy missing now fetches the packument again into storage, ignoring that row, and reads it once more, for packument, version and tarball requests. --- internal/handler/metadata_stored.go | 28 +++++- internal/handler/npm.go | 149 +++++++++++++++++----------- internal/handler/npm_stream_test.go | 87 +++++++++++----- 3 files changed, 176 insertions(+), 88 deletions(-) diff --git a/internal/handler/metadata_stored.go b/internal/handler/metadata_stored.go index 80cbaf46..7d43d011 100644 --- a/internal/handler/metadata_stored.go +++ b/internal/handler/metadata_stored.go @@ -45,13 +45,27 @@ var errStoredMetadataChanged = errors.New("stored metadata changed while it was // It returns errMetadataNotStored when the proxy doesn't cache metadata or // storing the document failed, and the caller reads it into memory instead. func (p *Proxy) storedMetadataDocument(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, validate func(io.Reader) error) (*database.MetadataCacheEntry, error) { + return p.storedMetadata(ctx, ecosystem, cacheKey, upstreamURL, accept, validate, "") +} + +// restoreStoredMetadata is storedMetadataDocument for a document whose row +// gone describes but whose stored copy was missing when read: the row is not +// served or revalidated, within its TTL or not, and the document is fetched +// again. +func (p *Proxy) restoreStoredMetadata(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, validate func(io.Reader) error, gone *database.MetadataCacheEntry) (*database.MetadataCacheEntry, error) { + return p.storedMetadata(ctx, ecosystem, cacheKey, upstreamURL, accept, validate, gone.ContentDigest.String) +} + +// storedMetadata implements storedMetadataDocument. A row with the digest +// gone is treated as if there were none. +func (p *Proxy) storedMetadata(ctx context.Context, ecosystem, cacheKey, upstreamURL, accept string, validate func(io.Reader) error, gone string) (*database.MetadataCacheEntry, error) { if !p.CacheMetadata || p.DB == nil || p.Storage == nil || p.MetadataTTL <= 0 { return nil, errMetadataNotStored } if containsPathTraversal(cacheKey) { return nil, fmt.Errorf("invalid cache key: %q", cacheKey) } - if entry, fresh := p.storedMetadataEntryState(ctx, ecosystem, cacheKey); fresh { + if entry, fresh := p.storedMetadataEntryState(ctx, ecosystem, cacheKey, gone); fresh { metrics.RecordMetadataCacheHit(ecosystem, int(entry.Size.Int64)) p.recordMetadataRead(entry) return entry, nil @@ -60,12 +74,12 @@ func (p *Proxy) storedMetadataDocument(ctx context.Context, ecosystem, cacheKey, key := metadataCoalesceKey(ecosystem, cacheKey, upstreamURL, accept, "", false) + "\x00stored" res, _ := p.coalesceMetadata(ctx, key, func(fetchCtx context.Context) metadataResult { - entry, fresh := p.storedMetadataEntryState(fetchCtx, ecosystem, cacheKey) + entry, fresh := p.storedMetadataEntryState(fetchCtx, ecosystem, cacheKey, gone) if fresh { return metadataResult{entry: entry} } release, outcome, err := p.awaitFetchLease(fetchCtx, metrics.CacheKindMetadata, key, func() bool { - entry, fresh = p.storedMetadataEntryState(fetchCtx, ecosystem, cacheKey) + entry, fresh = p.storedMetadataEntryState(fetchCtx, ecosystem, cacheKey, gone) return fresh }) if err != nil { @@ -92,10 +106,14 @@ func (p *Proxy) storedMetadataDocument(ctx context.Context, ecosystem, cacheKey, } // storedMetadataEntryState reads the cache row of a document and reports -// whether it is within its TTL and usable by storedMetadataDocument. -func (p *Proxy) storedMetadataEntryState(ctx context.Context, ecosystem, cacheKey string) (*database.MetadataCacheEntry, bool) { +// whether it is within its TTL and usable by storedMetadataDocument. A row +// with the digest gone is returned as nil. +func (p *Proxy) storedMetadataEntryState(ctx context.Context, ecosystem, cacheKey, gone string) (*database.MetadataCacheEntry, bool) { entry, _ := p.DB.GetMetadataCache(ecosystem, cacheKey) entry = p.migrateLegacyMetadataEntry(ctx, ecosystem, cacheKey, entry) + if gone != "" && entry != nil && entry.ContentDigest.String == gone { + return nil, false + } fresh := usableStoredMetadata(entry) && entry.FetchedAt.Valid && time.Since(entry.FetchedAt.Time) < p.MetadataTTL return entry, fresh } diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 05e77eaf..054400b3 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -17,6 +17,7 @@ import ( "github.com/git-pkgs/proxy/internal/jsonscan" "github.com/git-pkgs/proxy/internal/metadata/npm" "github.com/git-pkgs/proxy/internal/retention" + "github.com/git-pkgs/proxy/internal/storage" ) const ( @@ -340,35 +341,36 @@ func (h *NPMHandler) serveStreamedPackument(w http.ResponseWriter, r *http.Reque if h.proxy.rewrites == nil { return false } - entry, err := h.storedPackument(r.Context(), upstreamURL, accept, metadataCacheKey) - switch { - case err == nil: - case errors.Is(err, errInMemory): - return false - default: - h.writeFetchError(w, err) - return true - } - fullRequired := h.packumentValidator() != nil options := h.npmRewriteOptions(packageName, nil) - rewritten, err := h.proxy.streamedRewrite(r.Context(), "npm", h.proxyURL, metadataCacheKey, entry, func(open func() (io.ReadCloser, error), out io.Writer) error { - var scan *npm.Scan - err := readStored(open, func(in io.Reader) (err error) { - scan, err = npm.ScanPackument(in) - return err - }) - if err != nil { - return err - } - if fullRequired && scan.Abbreviated() { - return npm.ErrNotStreamable - } - return readStored(open, func(in io.Reader) error { - _, err := npm.RewriteStream(packageName, scan, in, out, options) - return err + var rewritten rewrittenMetadata + err, fetchErr := h.readStoredPackument(r.Context(), upstreamURL, accept, metadataCacheKey, func(entry *database.MetadataCacheEntry) (err error) { + rewritten, err = h.proxy.streamedRewrite(r.Context(), "npm", h.proxyURL, metadataCacheKey, entry, func(open func() (io.ReadCloser, error), out io.Writer) error { + var scan *npm.Scan + err := readStored(open, func(in io.Reader) (err error) { + scan, err = npm.ScanPackument(in) + return err + }) + if err != nil { + return err + } + if fullRequired && scan.Abbreviated() { + return npm.ErrNotStreamable + } + return readStored(open, func(in io.Reader) error { + _, err := npm.RewriteStream(packageName, scan, in, out, options) + return err + }) }) + return err }) + switch { + case errors.Is(fetchErr, errInMemory): + return false + case fetchErr != nil: + h.writeFetchError(w, fetchErr) + return true + } if err != nil { if r.Context().Err() != nil { return true // the client left while waiting on a shared rewrite @@ -419,6 +421,30 @@ func (h *NPMHandler) storedPackument(ctx context.Context, packumentURL, accept, return entry, err } +// readStoredPackument reads the packument stored for cacheKey with read, +// fetching it into storage first when needed. When the stored copy is gone, +// although its row says otherwise, the packument is fetched again and read +// once more. fetchErr is what storedPackument returned when the packument +// couldn't be fetched, and err is what read returned. +func (h *NPMHandler) readStoredPackument(ctx context.Context, packumentURL, accept, cacheKey string, read func(*database.MetadataCacheEntry) error) (err, fetchErr error) { + entry, fetchErr := h.storedPackument(ctx, packumentURL, accept, cacheKey) + if fetchErr != nil { + return nil, fetchErr + } + if err = read(entry); !errors.Is(err, storage.ErrNotFound) { + return err, nil + } + h.proxy.Logger.Warn("stored npm metadata is missing, fetching it again", "url", packumentURL) + entry, fetchErr = h.proxy.restoreStoredMetadata(ctx, "npm", cacheKey, packumentURL, accept, h.packumentStreamValidator(), entry) + if errors.Is(fetchErr, errMetadataNotStored) { + return nil, errInMemory + } + if fetchErr != nil { + return nil, fetchErr + } + return read(entry), nil +} + // packumentStreamValidator is packumentValidator for a packument read from // storage. func (h *NPMHandler) packumentStreamValidator() func(io.Reader) error { @@ -446,37 +472,40 @@ func (h *NPMHandler) streamedVersionMetadata(ctx context.Context, packageName, s if h.proxy.rewrites == nil { return rewrittenMetadata{}, errInMemory } - entry, err := h.storedPackument(ctx, packumentURL, accept, cacheKey) - if errors.Is(err, errInMemory) { - return rewrittenMetadata{}, err - } - if err != nil { - return rewrittenMetadata{}, &npmFetchError{err} - } fullRequired := h.packumentValidator() != nil options := h.npmRewriteOptions(packageName, nil) - m, err := h.proxy.streamedRewrite(ctx, "npm", h.proxyURL, metadataCacheKey, entry, func(open func() (io.ReadCloser, error), out io.Writer) error { - var scan *npm.Scan - err := readStored(open, func(in io.Reader) (err error) { - scan, err = npm.ScanPackument(in) - return err - }) - if err != nil { - return err - } - if fullRequired && scan.Abbreviated() { - return npm.ErrNotStreamable - } - return readStored(open, func(in io.Reader) error { - version, err := npm.VersionStream(packageName, specifier, scan, in, options) + var m rewrittenMetadata + err, fetchErr := h.readStoredPackument(ctx, packumentURL, accept, cacheKey, func(entry *database.MetadataCacheEntry) (err error) { + m, err = h.proxy.streamedRewrite(ctx, "npm", h.proxyURL, metadataCacheKey, entry, func(open func() (io.ReadCloser, error), out io.Writer) error { + var scan *npm.Scan + err := readStored(open, func(in io.Reader) (err error) { + scan, err = npm.ScanPackument(in) + return err + }) if err != nil { - return npmRewriteError(err) + return err } - _, err = out.Write(version) - return err + if fullRequired && scan.Abbreviated() { + return npm.ErrNotStreamable + } + return readStored(open, func(in io.Reader) error { + version, err := npm.VersionStream(packageName, specifier, scan, in, options) + if err != nil { + return npmRewriteError(err) + } + _, err = out.Write(version) + return err + }) }) + return err }) switch { + case errors.Is(fetchErr, errInMemory): + return rewrittenMetadata{}, fetchErr + case fetchErr != nil: + return rewrittenMetadata{}, &npmFetchError{fetchErr} + } + switch { case err == nil, ctx.Err() != nil, errors.Is(err, ErrVersionDenied), errors.Is(err, ErrVersionNotFound): return m, err case !errors.Is(err, npm.ErrNotStreamable): @@ -856,20 +885,20 @@ type npmVersionData struct { // otherwise from the packument read into memory. func (h *NPMHandler) versionData(ctx context.Context, packumentURL, cacheKey, version string) (npmVersionData, error) { accept := h.packumentAccept() - if entry, err := h.storedPackument(ctx, packumentURL, accept, cacheKey); err == nil { - var found npm.VersionEntry - err := readStored(func() (io.ReadCloser, error) { return h.proxy.openStoredMetadata(ctx, entry) }, func(in io.Reader) (err error) { + var found npm.VersionEntry + err, fetchErr := h.readStoredPackument(ctx, packumentURL, accept, cacheKey, func(entry *database.MetadataCacheEntry) error { + return readStored(func() (io.ReadCloser, error) { return h.proxy.openStoredMetadata(ctx, entry) }, func(in io.Reader) (err error) { found, err = npm.LookupVersion(in, version) return err }) - switch { - case err == nil && (!found.Abbreviated || h.packumentValidator() == nil): - return npmVersionData{entry: found.Entry, published: found.Published}, nil - case err != nil: - h.proxy.Logger.Warn("failed to read stored npm metadata, reading it in memory", "url", packumentURL, "error", err) - } - } else if !errors.Is(err, errInMemory) { - return npmVersionData{}, err + }) + switch { + case fetchErr == nil && err == nil && (!found.Abbreviated || h.packumentValidator() == nil): + return npmVersionData{entry: found.Entry, published: found.Published}, nil + case fetchErr == nil && err != nil: + h.proxy.Logger.Warn("failed to read stored npm metadata, reading it in memory", "url", packumentURL, "error", err) + case fetchErr != nil && !errors.Is(fetchErr, errInMemory): + return npmVersionData{}, fetchErr } body, _, _, err := h.proxy.fetchOrCacheMetadata(ctx, "npm", cacheKey, packumentURL, "", h.packumentValidator(), accept) diff --git a/internal/handler/npm_stream_test.go b/internal/handler/npm_stream_test.go index de5a17d2..d63ab9b3 100644 --- a/internal/handler/npm_stream_test.go +++ b/internal/handler/npm_stream_test.go @@ -232,33 +232,74 @@ func TestNPMStreamedPackumentStaleOn404(t *testing.T) { } } -// TestNPMStreamedPackumentBlobMissing checks that a 304 for a row whose -// document is gone from storage fetches the document again, also when it is -// larger than metadata_max_size. +// TestNPMStreamedPackumentBlobMissing checks that a row whose document is +// gone from storage, within its TTL or revalidated by a 304 after it, fetches +// the document again, also when it is larger than metadata_max_size. func TestNPMStreamedPackumentBlobMissing(t *testing.T) { - body := bigPackument(400) - u := newStreamUpstream(t, body) - proxy, store, handler := streamProxy(t, u) - proxy.MetadataMaxSize = int64(len(body) / 4) - decodeStreamed(t, serve(handler, "/big", nil)) + for _, tt := range []struct { + name string + ttl time.Duration + }{ + {"fresh row", time.Hour}, + {"expired row", time.Nanosecond}, + } { + t.Run(tt.name, func(t *testing.T) { + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, store, handler := streamProxy(t, u) + proxy.MetadataMaxSize = int64(len(body) / 4) + decodeStreamed(t, serve(handler, "/big", nil)) + + path := metadataStoragePath("npm", "big") + store.mu.Lock() + delete(store.files, path) + store.mu.Unlock() + proxy.MetadataTTL = tt.ttl + proxy.SetMetadataRewriteCacheSize(64 << 20) + + if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != 400 { + t.Errorf("served %d versions, want 400", len(doc.Versions)) + } + if n := u.sent.Load(); n != 2 { + t.Errorf("upstream sent the document %d times, want 2", n) + } + store.mu.Lock() + defer store.mu.Unlock() + if string(store.files[path]) != body { + t.Error("the document was not stored again") + } + }) + } +} +// TestNPMStreamedVersionBlobMissing checks that a version request and a +// tarball download fetch a packument again whose stored copy is gone while +// its row is within the TTL. +func TestNPMStreamedVersionBlobMissing(t *testing.T) { + u := newStreamUpstream(t, "") + u.body.Store(bigPackumentAt(u.URL, 100)) + proxy, store, handler := streamProxy(t, u) + proxy.MetadataMaxSize = 1024 + fetcher := proxy.Fetcher.(*mockFetcher) + fetcher.artifact = &fetch.Artifact{Body: io.NopCloser(strings.NewReader("package"))} path := metadataStoragePath("npm", "big") - store.mu.Lock() - delete(store.files, path) - store.mu.Unlock() - proxy.MetadataTTL = time.Nanosecond - proxy.SetMetadataRewriteCacheSize(64 << 20) - if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != 400 { - t.Errorf("served %d versions, want 400", len(doc.Versions)) - } - if n := u.sent.Load(); n != 2 { - t.Errorf("upstream sent the document %d times, want 2", n) - } - store.mu.Lock() - defer store.mu.Unlock() - if string(store.files[path]) != body { - t.Error("the document was not stored again") + for _, req := range []string{"/big/1.0.5", "/big/-/big-1.0.5.tgz"} { + decodeStreamed(t, serve(handler, "/big", nil)) + store.mu.Lock() + delete(store.files, path) + store.mu.Unlock() + proxy.SetMetadataRewriteCacheSize(64 << 20) + + if rec := serve(handler, req, nil); rec.Code != http.StatusOK { + t.Errorf("%s: status %d: %.200s", req, rec.Code, rec.Body.String()) + } + store.mu.Lock() + _, ok := store.files[path] + store.mu.Unlock() + if !ok { + t.Errorf("%s: the packument was not stored again", req) + } } } From f0c96e87a81ce6f2404ddd45c7527747c6d6dbaf Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 31 Oct 2026 17:50:24 +0100 Subject: [PATCH 6/8] Keep the stored copy when upstream cuts a metadata document off A body that ended before its Content-Length was read as complete in memory: ReadMetadataSized took the transport's io.ErrUnexpectedEOF for the end of a body shorter than its size hint. The half document was served and cached, or failed the filter with a 502, instead of the stored copy being served. Upstream bodies are now read through a reader that marks their read errors, and such a failure is an upstream failure on both paths: streamed, it no longer counts as a failure to store, which had sent the request to the bounded in-memory path. A stored packument whose bytes no longer match its row's digest is now fetched again like a missing one. Tests also cover concurrent requests sharing one fetch, client revalidation and upstream failures with nothing cached. The stream tests build cooldown through cooldown.NewPolicy as the server does, since a bare cooldown.Config parses itself on first use without locking. --- internal/handler/handler.go | 9 +- internal/handler/metadata_stored.go | 36 +++- internal/handler/npm.go | 10 +- internal/handler/npm_stream_harden_test.go | 206 +++++++++++++++++++++ internal/handler/npm_stream_test.go | 27 ++- internal/handler/read_metadata_test.go | 16 ++ 6 files changed, 287 insertions(+), 17 deletions(-) create mode 100644 internal/handler/npm_stream_harden_test.go diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 96ae655e..e0cede7d 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -157,7 +157,9 @@ func (p *Proxy) ReadMetadata(r io.Reader) ([]byte, error) { // Reading into a buffer of that size avoids io.ReadAll growing its buffer // step by step, which peaks at more than twice the document size. size is // only a hint: a negative one, or one above the limit, reads as ReadMetadata -// does, and a body of a different length is still read correctly. +// does, and a body of a different length is still read correctly. A body +// read through upstreamBodyReader that upstream cuts off fails, even though +// it ends like a body shorter than size. func (p *Proxy) ReadMetadataSized(r io.Reader, size int64) ([]byte, error) { limit := p.MetadataMaxSize if limit <= 0 { @@ -170,7 +172,10 @@ func (p *Proxy) ReadMetadataSized(r io.Reader, size int64) ([]byte, error) { // the buffer and a longer one is noticed. buf := make([]byte, size+1) n, err := io.ReadFull(r, buf) + var cut *upstreamBodyError switch { + case errors.As(err, &cut): + return nil, err case errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF): if int64(n) < size/2 { // The hint was far off; don't keep the oversized buffer alive. @@ -1989,7 +1994,7 @@ func (p *Proxy) fetchUpstreamMetadata(ctx context.Context, upstreamURL string, e // ContentLength is -1 when the transport decompresses the body, and // otherwise the length of the bytes read here. - body, err := p.ReadMetadataSized(resp.Body, resp.ContentLength) + body, err := p.ReadMetadataSized(upstreamBodyReader{resp.Body}, resp.ContentLength) if err != nil { return nil, fmt.Errorf("reading response: %w", err) } diff --git a/internal/handler/metadata_stored.go b/internal/handler/metadata_stored.go index 7d43d011..8bbf3ebf 100644 --- a/internal/handler/metadata_stored.go +++ b/internal/handler/metadata_stored.go @@ -200,9 +200,9 @@ func (p *Proxy) storeUpstreamMetadata(ctx context.Context, ecosystem, cacheKey, } // Go decompresses the body, since no Accept-Encoding was set, so it is // stored uncompressed. - var body io.Reader = resp.Body + var body io.Reader = upstreamBodyReader{resp.Body} if validate != nil { - staged, err := p.stageMetadata(ctx, ecosystem, cacheKey, resp.Body, validate) + staged, err := p.stageMetadata(ctx, ecosystem, cacheKey, body, validate) if err != nil { return nil, err } @@ -210,7 +210,7 @@ func (p *Proxy) storeUpstreamMetadata(ctx context.Context, ecosystem, cacheKey, body = staged } if _, err := p.storeMetadataBlob(ctx, ecosystem, cacheKey, metadataStoragePath(ecosystem, cacheKey), body, meta); err != nil { - return nil, fmt.Errorf("%w: %w", errMetadataNotStored, err) + return nil, notStored("", err) } stored, err := p.DB.GetMetadataCache(ecosystem, cacheKey) if err != nil || !usableStoredMetadata(stored) { @@ -232,7 +232,7 @@ func (p *Proxy) stageMetadata(ctx context.Context, ecosystem, cacheKey string, b } if _, _, err := p.Storage.Store(ctx, path, body); err != nil { discard() - return nil, fmt.Errorf("%w: staging: %w", errMetadataNotStored, err) + return nil, notStored("staging: ", err) } if err := readStored(func() (io.ReadCloser, error) { return p.Storage.Open(ctx, path) }, validate); err != nil { discard() @@ -246,6 +246,34 @@ func (p *Proxy) stageMetadata(ctx context.Context, ecosystem, cacheKey string, b return &stagedReader{ReadCloser: staged, discard: discard}, nil } +// upstreamBodyReader marks a failure to read a document from upstream, so +// it is told apart from a failure to store it. +type upstreamBodyReader struct{ r io.Reader } + +func (u upstreamBodyReader) Read(b []byte) (int, error) { + n, err := u.r.Read(b) + if err != nil && !errors.Is(err, io.EOF) { + err = &upstreamBodyError{err} + } + return n, err +} + +// upstreamBodyError is a failure to read a document from upstream. It is +// handled as an upstream failure: the stored copy, if any, is served. +type upstreamBodyError struct{ err error } + +func (e *upstreamBodyError) Error() string { return "reading upstream metadata: " + e.err.Error() } +func (e *upstreamBodyError) Unwrap() error { return e.err } + +// notStored returns a failure to store a document as errMetadataNotStored, +// unless reading it from upstream failed. +func notStored(what string, err error) error { + if body := (*upstreamBodyError)(nil); errors.As(err, &body) { + return body + } + return fmt.Errorf("%w: %s%w", errMetadataNotStored, what, err) +} + // stagedReader deletes the staged copy it reads when closed. type stagedReader struct { io.ReadCloser diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 054400b3..50757a37 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -422,19 +422,19 @@ func (h *NPMHandler) storedPackument(ctx context.Context, packumentURL, accept, } // readStoredPackument reads the packument stored for cacheKey with read, -// fetching it into storage first when needed. When the stored copy is gone, -// although its row says otherwise, the packument is fetched again and read -// once more. fetchErr is what storedPackument returned when the packument +// fetching it into storage first when needed. When the stored copy is gone +// or no longer has the digest its row records, the packument is fetched +// again and read once more. fetchErr is what storedPackument returned when the packument // couldn't be fetched, and err is what read returned. func (h *NPMHandler) readStoredPackument(ctx context.Context, packumentURL, accept, cacheKey string, read func(*database.MetadataCacheEntry) error) (err, fetchErr error) { entry, fetchErr := h.storedPackument(ctx, packumentURL, accept, cacheKey) if fetchErr != nil { return nil, fetchErr } - if err = read(entry); !errors.Is(err, storage.ErrNotFound) { + if err = read(entry); !errors.Is(err, storage.ErrNotFound) && !errors.Is(err, errStoredMetadataChanged) { return err, nil } - h.proxy.Logger.Warn("stored npm metadata is missing, fetching it again", "url", packumentURL) + h.proxy.Logger.Warn("stored npm metadata is missing or changed, fetching it again", "url", packumentURL, "error", err) entry, fetchErr = h.proxy.restoreStoredMetadata(ctx, "npm", cacheKey, packumentURL, accept, h.packumentStreamValidator(), entry) if errors.Is(fetchErr, errMetadataNotStored) { return nil, errInMemory diff --git a/internal/handler/npm_stream_harden_test.go b/internal/handler/npm_stream_harden_test.go new file mode 100644 index 00000000..ff8f97e4 --- /dev/null +++ b/internal/handler/npm_stream_harden_test.go @@ -0,0 +1,206 @@ +package handler + +import ( + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" +) + +// bigMetadataFiles returns a copy of what is in storage under the metadata path +// of big, staged copies included. +func bigMetadataFiles(store *mockStorage) map[string]string { + store.mu.Lock() + defer store.mu.Unlock() + files := map[string]string{} + for path, b := range store.files { + if strings.HasPrefix(path, metadataStoragePath("npm", "big")) { + files[path] = string(b) + } + } + return files +} + +// TestNPMStreamedPackumentBlobCorrupt checks that a stored copy whose bytes +// no longer match the digest of its row, within its TTL, is fetched again +// rather than failing the request, also above metadata_max_size. +func TestNPMStreamedPackumentBlobCorrupt(t *testing.T) { + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, store, handler := streamProxy(t, u) + proxy.MetadataMaxSize = int64(len(body) / 4) + decodeStreamed(t, serve(handler, "/big", nil)) + + path := metadataStoragePath("npm", "big") + store.mu.Lock() + store.files[path] = []byte(strings.Replace(body, "1.0.5", "1.0.X", 1)) + store.mu.Unlock() + proxy.SetMetadataRewriteCacheSize(64 << 20) + + if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != 400 { + t.Errorf("served %d versions, want 400", len(doc.Versions)) + } + if got := bigMetadataFiles(store)[path]; got != body { + t.Error("the corrupt copy was not replaced") + } +} + +// TestNPMStreamedPackumentTruncated checks that a document upstream cuts +// off halfway neither replaces the stored copy, which is served instead, +// nor leaves anything behind, with and without cooldown, streamed and in +// memory. +func TestNPMStreamedPackumentTruncated(t *testing.T) { + for _, tt := range []struct { + name string + cooldown, inMemory bool + }{ + {"cooldown", true, false}, + {"no cooldown", false, false}, + {"in memory with cooldown", true, true}, + {"in memory without cooldown", false, true}, + } { + t.Run(tt.name, func(t *testing.T) { + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, store, handler := streamProxy(t, u) + if !tt.cooldown { + proxy.Cooldown = nil + } + if tt.inMemory { + proxy.SetMetadataRewriteCacheSize(0) + } else { + proxy.MetadataMaxSize = int64(len(body) / 4) + } + want := len(decodeStreamed(t, serve(handler, "/big", nil)).Versions) + before := bigMetadataFiles(store) + + u.replace(bigPackument(401), `"v2"`) + u.truncate.Store(true) + proxy.MetadataTTL = time.Nanosecond + if !tt.inMemory { + proxy.SetMetadataRewriteCacheSize(64 << 20) + } + + if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != want { + t.Errorf("served %d versions, want the stored %d", len(doc.Versions), want) + } + after := bigMetadataFiles(store) + if len(after) != len(before) { + t.Errorf("storage holds %d files, %d before", len(after), len(before)) + } + for path, b := range before { + if after[path] != b { + t.Errorf("%s was changed", path) + } + } + }) + } +} + +// TestNPMStreamedPackumentTruncatedUncached checks that a document upstream +// cuts off halfway fails the request and is not kept, streamed and in memory. +func TestNPMStreamedPackumentTruncatedUncached(t *testing.T) { + for _, inMemory := range []bool{false, true} { + t.Run(map[bool]string{false: "streamed", true: "in memory"}[inMemory], func(t *testing.T) { + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, store, handler := streamProxy(t, u) + proxy.Cooldown = nil + if inMemory { + proxy.SetMetadataRewriteCacheSize(0) + } else { + proxy.MetadataMaxSize = int64(len(body) / 4) + } + u.truncate.Store(true) + + if rec := serve(handler, "/big", nil); rec.Code != http.StatusBadGateway { + t.Errorf("status %d, want 502", rec.Code) + } + if files := bigMetadataFiles(store); len(files) != 0 { + t.Errorf("left %d files in storage", len(files)) + } + if entry, _ := proxy.DB.GetMetadataCache("npm", "big"); entry != nil { + t.Error("left a metadata cache row") + } + }) + } +} + +// TestNPMStreamedPackumentUncachedErrors checks the answers to upstream +// failures with nothing cached. +func TestNPMStreamedPackumentUncachedErrors(t *testing.T) { + for _, tt := range []struct { + name string + set func(*streamUpstream) + want int + }{ + {"not found", func(u *streamUpstream) { u.gone.Store(true) }, http.StatusNotFound}, + {"upstream down", func(u *streamUpstream) { u.fail.Store(true) }, http.StatusBadGateway}, + } { + t.Run(tt.name, func(t *testing.T) { + u := newStreamUpstream(t, bigPackument(10)) + _, store, handler := streamProxy(t, u) + tt.set(u) + if rec := serve(handler, "/big", nil); rec.Code != tt.want { + t.Errorf("status %d, want %d", rec.Code, tt.want) + } + if files := bigMetadataFiles(store); len(files) != 0 { + t.Errorf("left %d files in storage", len(files)) + } + }) + } +} + +// TestNPMStreamedPackumentConcurrent checks that concurrent requests for an +// uncached packument share one upstream fetch and all get it. +func TestNPMStreamedPackumentConcurrent(t *testing.T) { + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, _, handler := streamProxy(t, u) + proxy.MetadataMaxSize = int64(len(body) / 4) + const n = 16 + var wg sync.WaitGroup + recs := make([]*httptest.ResponseRecorder, n) + for i := range n { + wg.Go(func() { recs[i] = serve(handler, "/big", nil) }) + } + wg.Wait() + for _, rec := range recs { + if doc := decodeStreamed(t, rec); len(doc.Versions) != 400 { + t.Errorf("served %d versions, want 400", len(doc.Versions)) + } + } + if got := u.sent.Load(); got != 1 { + t.Errorf("upstream sent the document %d times, want once", got) + } +} + +// TestNPMStreamedPackumentClientRevalidates checks that a client sending +// the ETag it got gets a 304, and a new ETag once the document changes. +func TestNPMStreamedPackumentClientRevalidates(t *testing.T) { + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, _, handler := streamProxy(t, u) + proxy.MetadataMaxSize = int64(len(body) / 4) + + first := serve(handler, "/big", nil) + etag := first.Header().Get("ETag") + if first.Code != http.StatusOK || etag == "" { + t.Fatalf("status %d, ETag %q", first.Code, etag) + } + if rec := serve(handler, "/big", http.Header{"If-None-Match": {etag}}); rec.Code != http.StatusNotModified { + t.Errorf("same document: status %d, want 304", rec.Code) + } + + u.replace(bigPackument(401), `"v2"`) + proxy.MetadataTTL = time.Nanosecond + rec := serve(handler, "/big", http.Header{"If-None-Match": {etag}}) + if rec.Code != http.StatusOK || rec.Header().Get("ETag") == etag { + t.Errorf("changed document: status %d, ETag %q (was %q)", rec.Code, rec.Header().Get("ETag"), etag) + } + if doc := decodeStreamed(t, rec); len(doc.Versions) != 401 { + t.Errorf("served %d versions, want 401", len(doc.Versions)) + } +} diff --git a/internal/handler/npm_stream_test.go b/internal/handler/npm_stream_test.go index d63ab9b3..a8a4ea2a 100644 --- a/internal/handler/npm_stream_test.go +++ b/internal/handler/npm_stream_test.go @@ -49,10 +49,13 @@ func bigPackumentAt(base string, n int) string { type streamUpstream struct { *httptest.Server - body atomic.Value // string - etag atomic.Value // string - fail atomic.Bool - gone atomic.Bool + body atomic.Value // string + etag atomic.Value // string + fail atomic.Bool + gone atomic.Bool + // truncate makes upstream close the connection halfway through the + // document it announced. + truncate atomic.Bool requests atomic.Int32 // sent counts the responses that carried the document. sent atomic.Int32 @@ -80,9 +83,15 @@ func newStreamUpstream(t *testing.T, body string) *streamUpstream { w.WriteHeader(http.StatusNotModified) case r.URL.Path == "/big": u.sent.Add(1) + body := u.body.Load().(string) w.Header().Set("Content-Type", "application/json") w.Header().Set("ETag", u.etag.Load().(string)) - _, _ = io.WriteString(w, u.body.Load().(string)) + if u.truncate.Load() { + w.Header().Set("Content-Length", fmt.Sprint(len(body))) + _, _ = io.WriteString(w, body[:len(body)/2]) + return + } + _, _ = io.WriteString(w, body) default: http.NotFound(w, r) } @@ -98,7 +107,13 @@ func streamProxy(t *testing.T, u *streamUpstream) (*Proxy, *mockStorage, http.Ha proxy.CacheMetadata = true proxy.MetadataTTL = time.Hour proxy.SetMetadataRewriteCacheSize(64 << 20) - proxy.Cooldown = &cooldown.Config{Default: "3d"} + // As the server does: a cooldown.Config parses itself on first use, + // which concurrent requests would race on. + policy, err := cooldown.NewPolicy(&cooldown.Config{Default: "3d"}, nil) + if err != nil { + t.Fatal(err) + } + proxy.Cooldown = policy return proxy, store, NewNPMHandler(proxy, "http://proxy.example", u.URL).Routes() } diff --git a/internal/handler/read_metadata_test.go b/internal/handler/read_metadata_test.go index 639405c8..d87d4169 100644 --- a/internal/handler/read_metadata_test.go +++ b/internal/handler/read_metadata_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "runtime" + "strings" "testing" "testing/iotest" ) @@ -131,3 +132,18 @@ func TestReadMetadataSizedAllocatesOnce(t *testing.T) { t.Errorf("allocated %d bytes reading %d", allocated, size) } } + +// TestReadMetadataSizedTruncatedUpstream checks that an upstream body cut +// off before its Content-Length fails, while a body shorter than a size +// hint is still read. +func TestReadMetadataSizedTruncatedUpstream(t *testing.T) { + p := &Proxy{} + cut := io.MultiReader(strings.NewReader(`{"name":`), iotest.ErrReader(io.ErrUnexpectedEOF)) + if _, err := p.ReadMetadataSized(upstreamBodyReader{cut}, 100); !errors.Is(err, io.ErrUnexpectedEOF) { + t.Errorf("cut off body: error %v, want io.ErrUnexpectedEOF", err) + } + got, err := p.ReadMetadataSized(strings.NewReader(`{"name":"x"}`), 100) + if err != nil || string(got) != `{"name":"x"}` { + t.Errorf("short body = %q, %v", got, err) + } +} From 6dbda21faf5092e663029d5fcb8c59ee5adb4654 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 1 Nov 2026 12:49:09 +0100 Subject: [PATCH 7/8] Move the in-memory truncation fix to its own pull request The change to ReadMetadataSized and fetchUpstreamMetadata, with its tests, is now #649. upstreamBodyReader stays for storeUpstreamMetadata, in a file of its own that #649 adds with the same content. --- internal/handler/handler.go | 9 +-- internal/handler/metadata_stored.go | 19 ------- internal/handler/npm_stream_harden_test.go | 65 +++++++--------------- internal/handler/read_metadata_test.go | 16 ------ internal/handler/upstream_body.go | 26 +++++++++ 5 files changed, 49 insertions(+), 86 deletions(-) create mode 100644 internal/handler/upstream_body.go diff --git a/internal/handler/handler.go b/internal/handler/handler.go index e0cede7d..96ae655e 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -157,9 +157,7 @@ func (p *Proxy) ReadMetadata(r io.Reader) ([]byte, error) { // Reading into a buffer of that size avoids io.ReadAll growing its buffer // step by step, which peaks at more than twice the document size. size is // only a hint: a negative one, or one above the limit, reads as ReadMetadata -// does, and a body of a different length is still read correctly. A body -// read through upstreamBodyReader that upstream cuts off fails, even though -// it ends like a body shorter than size. +// does, and a body of a different length is still read correctly. func (p *Proxy) ReadMetadataSized(r io.Reader, size int64) ([]byte, error) { limit := p.MetadataMaxSize if limit <= 0 { @@ -172,10 +170,7 @@ func (p *Proxy) ReadMetadataSized(r io.Reader, size int64) ([]byte, error) { // the buffer and a longer one is noticed. buf := make([]byte, size+1) n, err := io.ReadFull(r, buf) - var cut *upstreamBodyError switch { - case errors.As(err, &cut): - return nil, err case errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF): if int64(n) < size/2 { // The hint was far off; don't keep the oversized buffer alive. @@ -1994,7 +1989,7 @@ func (p *Proxy) fetchUpstreamMetadata(ctx context.Context, upstreamURL string, e // ContentLength is -1 when the transport decompresses the body, and // otherwise the length of the bytes read here. - body, err := p.ReadMetadataSized(upstreamBodyReader{resp.Body}, resp.ContentLength) + body, err := p.ReadMetadataSized(resp.Body, resp.ContentLength) if err != nil { return nil, fmt.Errorf("reading response: %w", err) } diff --git a/internal/handler/metadata_stored.go b/internal/handler/metadata_stored.go index 8bbf3ebf..b3bc4202 100644 --- a/internal/handler/metadata_stored.go +++ b/internal/handler/metadata_stored.go @@ -246,25 +246,6 @@ func (p *Proxy) stageMetadata(ctx context.Context, ecosystem, cacheKey string, b return &stagedReader{ReadCloser: staged, discard: discard}, nil } -// upstreamBodyReader marks a failure to read a document from upstream, so -// it is told apart from a failure to store it. -type upstreamBodyReader struct{ r io.Reader } - -func (u upstreamBodyReader) Read(b []byte) (int, error) { - n, err := u.r.Read(b) - if err != nil && !errors.Is(err, io.EOF) { - err = &upstreamBodyError{err} - } - return n, err -} - -// upstreamBodyError is a failure to read a document from upstream. It is -// handled as an upstream failure: the stored copy, if any, is served. -type upstreamBodyError struct{ err error } - -func (e *upstreamBodyError) Error() string { return "reading upstream metadata: " + e.err.Error() } -func (e *upstreamBodyError) Unwrap() error { return e.err } - // notStored returns a failure to store a document as errMetadataNotStored, // unless reading it from upstream failed. func notStored(what string, err error) error { diff --git a/internal/handler/npm_stream_harden_test.go b/internal/handler/npm_stream_harden_test.go index ff8f97e4..ca7143fe 100644 --- a/internal/handler/npm_stream_harden_test.go +++ b/internal/handler/npm_stream_harden_test.go @@ -49,39 +49,24 @@ func TestNPMStreamedPackumentBlobCorrupt(t *testing.T) { // TestNPMStreamedPackumentTruncated checks that a document upstream cuts // off halfway neither replaces the stored copy, which is served instead, -// nor leaves anything behind, with and without cooldown, streamed and in -// memory. +// nor leaves anything behind, with and without cooldown. func TestNPMStreamedPackumentTruncated(t *testing.T) { - for _, tt := range []struct { - name string - cooldown, inMemory bool - }{ - {"cooldown", true, false}, - {"no cooldown", false, false}, - {"in memory with cooldown", true, true}, - {"in memory without cooldown", false, true}, - } { - t.Run(tt.name, func(t *testing.T) { + for _, withCooldown := range []bool{true, false} { + t.Run(map[bool]string{true: "cooldown", false: "no cooldown"}[withCooldown], func(t *testing.T) { body := bigPackument(400) u := newStreamUpstream(t, body) proxy, store, handler := streamProxy(t, u) - if !tt.cooldown { + if !withCooldown { proxy.Cooldown = nil } - if tt.inMemory { - proxy.SetMetadataRewriteCacheSize(0) - } else { - proxy.MetadataMaxSize = int64(len(body) / 4) - } + proxy.MetadataMaxSize = int64(len(body) / 4) want := len(decodeStreamed(t, serve(handler, "/big", nil)).Versions) before := bigMetadataFiles(store) u.replace(bigPackument(401), `"v2"`) u.truncate.Store(true) proxy.MetadataTTL = time.Nanosecond - if !tt.inMemory { - proxy.SetMetadataRewriteCacheSize(64 << 20) - } + proxy.SetMetadataRewriteCacheSize(64 << 20) if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != want { t.Errorf("served %d versions, want the stored %d", len(doc.Versions), want) @@ -100,31 +85,23 @@ func TestNPMStreamedPackumentTruncated(t *testing.T) { } // TestNPMStreamedPackumentTruncatedUncached checks that a document upstream -// cuts off halfway fails the request and is not kept, streamed and in memory. +// cuts off halfway fails the request and is not kept. func TestNPMStreamedPackumentTruncatedUncached(t *testing.T) { - for _, inMemory := range []bool{false, true} { - t.Run(map[bool]string{false: "streamed", true: "in memory"}[inMemory], func(t *testing.T) { - body := bigPackument(400) - u := newStreamUpstream(t, body) - proxy, store, handler := streamProxy(t, u) - proxy.Cooldown = nil - if inMemory { - proxy.SetMetadataRewriteCacheSize(0) - } else { - proxy.MetadataMaxSize = int64(len(body) / 4) - } - u.truncate.Store(true) + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, store, handler := streamProxy(t, u) + proxy.Cooldown = nil + proxy.MetadataMaxSize = int64(len(body) / 4) + u.truncate.Store(true) - if rec := serve(handler, "/big", nil); rec.Code != http.StatusBadGateway { - t.Errorf("status %d, want 502", rec.Code) - } - if files := bigMetadataFiles(store); len(files) != 0 { - t.Errorf("left %d files in storage", len(files)) - } - if entry, _ := proxy.DB.GetMetadataCache("npm", "big"); entry != nil { - t.Error("left a metadata cache row") - } - }) + if rec := serve(handler, "/big", nil); rec.Code != http.StatusBadGateway { + t.Errorf("status %d, want 502", rec.Code) + } + if files := bigMetadataFiles(store); len(files) != 0 { + t.Errorf("left %d files in storage", len(files)) + } + if entry, _ := proxy.DB.GetMetadataCache("npm", "big"); entry != nil { + t.Error("left a metadata cache row") } } diff --git a/internal/handler/read_metadata_test.go b/internal/handler/read_metadata_test.go index d87d4169..639405c8 100644 --- a/internal/handler/read_metadata_test.go +++ b/internal/handler/read_metadata_test.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "runtime" - "strings" "testing" "testing/iotest" ) @@ -132,18 +131,3 @@ func TestReadMetadataSizedAllocatesOnce(t *testing.T) { t.Errorf("allocated %d bytes reading %d", allocated, size) } } - -// TestReadMetadataSizedTruncatedUpstream checks that an upstream body cut -// off before its Content-Length fails, while a body shorter than a size -// hint is still read. -func TestReadMetadataSizedTruncatedUpstream(t *testing.T) { - p := &Proxy{} - cut := io.MultiReader(strings.NewReader(`{"name":`), iotest.ErrReader(io.ErrUnexpectedEOF)) - if _, err := p.ReadMetadataSized(upstreamBodyReader{cut}, 100); !errors.Is(err, io.ErrUnexpectedEOF) { - t.Errorf("cut off body: error %v, want io.ErrUnexpectedEOF", err) - } - got, err := p.ReadMetadataSized(strings.NewReader(`{"name":"x"}`), 100) - if err != nil || string(got) != `{"name":"x"}` { - t.Errorf("short body = %q, %v", got, err) - } -} diff --git a/internal/handler/upstream_body.go b/internal/handler/upstream_body.go new file mode 100644 index 00000000..7db90765 --- /dev/null +++ b/internal/handler/upstream_body.go @@ -0,0 +1,26 @@ +package handler + +import ( + "errors" + "io" +) + +// upstreamBodyReader marks a failure to read a document from upstream, so a +// body upstream cuts off is told apart from one that ends, and from a +// failure to store it. +type upstreamBodyReader struct{ r io.Reader } + +func (u upstreamBodyReader) Read(b []byte) (int, error) { + n, err := u.r.Read(b) + if err != nil && !errors.Is(err, io.EOF) { + err = &upstreamBodyError{err} + } + return n, err +} + +// upstreamBodyError is a failure to read a document from upstream. It is +// handled as an upstream failure: the stored copy, if any, is served. +type upstreamBodyError struct{ err error } + +func (e *upstreamBodyError) Error() string { return "reading upstream metadata: " + e.err.Error() } +func (e *upstreamBodyError) Unwrap() error { return e.err } From b08cebf9e4d45f723f4e39b9c5d6bf94f596427d Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sun, 1 Nov 2026 12:53:00 +0100 Subject: [PATCH 8/8] Cap npm packuments streamed into storage metadata_max_size bounds what is read into memory, but a packument streamed from upstream into storage had no limit, and with cooldown was stored twice. It is now capped by metadata_stream_max_size, 1 GB by default. A document over the cap fails as an upstream failure: nothing is stored, and the stored copy, if any, is served instead. The metadata settings move into configureMetadata to keep serve under the maintidx limit. --- cmd/proxy/main.go | 1 + docs/configuration.md | 7 +-- internal/config/config.go | 31 ++++++++++-- internal/config/config_test.go | 23 +++++++++ internal/handler/handler.go | 23 +++++---- internal/handler/metadata_stored.go | 21 ++++++++- internal/handler/npm_stream_harden_test.go | 55 ++++++++++++++++++++++ internal/server/server.go | 14 ++++-- 8 files changed, 155 insertions(+), 20 deletions(-) diff --git a/cmd/proxy/main.go b/cmd/proxy/main.go index 824f2e2c..d39125d5 100644 --- a/cmd/proxy/main.go +++ b/cmd/proxy/main.go @@ -561,6 +561,7 @@ func runMirror() { proxy.CacheMetadata = true // mirror always caches metadata proxy.MetadataTTL = cfg.ParseMetadataTTL() proxy.MetadataMaxSize = cfg.ParseMetadataMaxSize() + proxy.MetadataStreamMaxSize = cfg.ParseMetadataStreamMaxSize() m := newCommandMirror(cfg, proxy, images, db, store, logger, *concurrency) switch s := source.(type) { diff --git a/docs/configuration.md b/docs/configuration.md index 93a7299f..5c33a6e2 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1403,13 +1403,14 @@ When upstream is unreachable and the cached entry is past its TTL, the proxy ser ### Metadata size limit -Upstream metadata responses are buffered in memory before being rewritten and served. `metadata_max_size` caps that buffer to protect against OOM from a misbehaving upstream. Some npm packages with thousands of versions (for example `renovate`) exceed the 100 MB default, so raise this if you see `metadata response exceeds size limit` in the logs. Conda `repodata.json` with cooldown enabled is filtered as it streams and isn't held in memory decompressed. With `cache_metadata` the limit applies to the compressed document as it does without cooldown; one over it is filtered as it streams from the upstream and not cached. With `cache_metadata` and the metadata rewrite cache on, an npm packument is streamed from the upstream into storage and rewritten from there in two passes, so it isn't held in memory and the limit doesn't apply to it; the rewritten copy is kept gzip-compressed. Requests for a single version (`//`) still read the packument into memory. +Upstream metadata responses are buffered in memory before being rewritten and served. `metadata_max_size` caps that buffer to protect against OOM from a misbehaving upstream. Some npm packages with thousands of versions (for example `renovate`) exceed the 100 MB default, so raise this if you see `metadata response exceeds size limit` in the logs. Conda `repodata.json` with cooldown enabled is filtered as it streams and isn't held in memory decompressed. With `cache_metadata` the limit applies to the compressed document as it does without cooldown; one over it is filtered as it streams from the upstream and not cached. With `cache_metadata` and the metadata rewrite cache on, an npm packument is streamed from the upstream into storage and rewritten from there in two passes, so it isn't held in memory and the limit doesn't apply to it; the rewritten copy is kept gzip-compressed. Requests for a single version (`//`) and tarball downloads read the version from the stored copy the same way. `metadata_stream_max_size` (1 GB by default) caps a document streamed this way; one over it isn't stored, and the stored copy, if any, is served instead. ```yaml -metadata_max_size: "100MB" # default +metadata_max_size: "100MB" # default +metadata_stream_max_size: "1GB" # default ``` -Or via environment variable: `PROXY_METADATA_MAX_SIZE=250MB`. +Or via environment variable: `PROXY_METADATA_MAX_SIZE=250MB`, and `PROXY_METADATA_STREAM_MAX_SIZE=2GB` for `metadata_stream_max_size`. ### Artifact size limit diff --git a/internal/config/config.go b/internal/config/config.go index 5e901505..14790a97 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -169,6 +169,11 @@ type Config struct { // size return ErrMetadataTooLarge. Default: "100MB". MetadataMaxSize string `json:"metadata_max_size" yaml:"metadata_max_size"` + // MetadataStreamMaxSize is the largest metadata document the proxy + // streams from upstream into storage instead of buffering it, which + // metadata_max_size doesn't cover (e.g. "1GB"). Default: "1GB". + MetadataStreamMaxSize string `json:"metadata_stream_max_size" yaml:"metadata_stream_max_size"` + // ArtifactMaxSize is the largest upstream artifact the proxy will fetch // (e.g. "2GB"). Larger artifacts are refused and not cached. Default: "" // (unlimited). "0" is also unlimited. @@ -1480,6 +1485,7 @@ func (c *Config) LoadFromEnv() { setEnvStringSlice(&c.Hosted.ComposerVendors, "PROXY_HOSTED_COMPOSER_VENDORS") setEnvString(&c.MetadataTTL, "PROXY_METADATA_TTL") setEnvString(&c.MetadataMaxSize, "PROXY_METADATA_MAX_SIZE") + setEnvString(&c.MetadataStreamMaxSize, "PROXY_METADATA_STREAM_MAX_SIZE") setEnvString(&c.ArtifactMaxSize, "PROXY_ARTIFACT_MAX_SIZE") setEnvString(&c.MetadataRewriteCacheSize, "PROXY_METADATA_REWRITE_CACHE_SIZE") setEnvString(&c.HTTPTimeout, "PROXY_HTTP_TIMEOUT") @@ -1726,6 +1732,7 @@ const ( defaultHTTPTimeout = 30 * time.Second //nolint:mnd // sensible default defaultHitFlushInterval = time.Second defaultMetadataMaxSize = 100 << 20 + defaultMetadataStreamMaxSize = 1 << 30 defaultMetadataRewriteCacheSize = 256 << 20 defaultGradleBuildCacheMaxUploadSize = 100 << 20 defaultGradleBuildCacheSweepInterval = 10 * time.Minute @@ -1779,22 +1786,25 @@ func validateRequestTimeout(field, s string) error { return nil } -func validateMetadataMaxSize(s string) error { +func validateMetadataMaxSize(name, s string) error { if s == "" { return nil } size, err := ParseSize(s) if err != nil { - return fmt.Errorf("invalid metadata_max_size: %w", err) + return fmt.Errorf("invalid %s: %w", name, err) } if size <= 0 { - return fmt.Errorf("invalid metadata_max_size %q: must be positive", s) + return fmt.Errorf("invalid %s %q: must be positive", name, s) } return nil } func (c *Config) validateDownloadSizeLimits() error { - if err := validateMetadataMaxSize(c.MetadataMaxSize); err != nil { + if err := validateMetadataMaxSize("metadata_max_size", c.MetadataMaxSize); err != nil { + return err + } + if err := validateMetadataMaxSize("metadata_stream_max_size", c.MetadataStreamMaxSize); err != nil { return err } return validateArtifactMaxSize(c.ArtifactMaxSize) @@ -1857,6 +1867,19 @@ func (c *Config) ParseMetadataMaxSize() int64 { return size } +// ParseMetadataStreamMaxSize returns the size limit for metadata streamed +// into storage in bytes. Returns 1GB if unset or invalid. +func (c *Config) ParseMetadataStreamMaxSize() int64 { + if c.MetadataStreamMaxSize == "" { + return defaultMetadataStreamMaxSize + } + size, err := ParseSize(c.MetadataStreamMaxSize) + if err != nil || size <= 0 { + return defaultMetadataStreamMaxSize + } + return size +} + // ParseArtifactMaxSize returns the artifact size limit in bytes, or 0 for // no limit. func (c *Config) ParseArtifactMaxSize() int64 { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index a0132cfd..00f45d95 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -785,6 +785,29 @@ func TestParseMetadataMaxSize(t *testing.T) { } } +func TestParseMetadataStreamMaxSize(t *testing.T) { + cfg := Default() + if got := cfg.ParseMetadataStreamMaxSize(); got != defaultMetadataStreamMaxSize { + t.Errorf("unset: %d, want %d", got, defaultMetadataStreamMaxSize) + } + cfg.MetadataStreamMaxSize = "2GB" + if got := cfg.ParseMetadataStreamMaxSize(); got != 2<<30 { + t.Errorf("2GB: %d", got) + } + t.Setenv("PROXY_METADATA_STREAM_MAX_SIZE", "300MB") + cfg = Default() + cfg.LoadFromEnv() + if got := cfg.ParseMetadataStreamMaxSize(); got != 300<<20 { + t.Errorf("from env: %d", got) + } + for _, bad := range []string{"0", "lots"} { + cfg.MetadataStreamMaxSize = bad + if err := cfg.Validate(); err == nil { + t.Errorf("metadata_stream_max_size %q validated", bad) + } + } +} + func TestValidateMetadataMaxSize(t *testing.T) { cfg := Default() cfg.MetadataMaxSize = "not-a-size" diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 96ae655e..832ac476 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -142,6 +142,10 @@ func ifNoneMatchHits(header, etag string) bool { // defaultMetadataMaxSize is used when Proxy.MetadataMaxSize is unset. const defaultMetadataMaxSize = 100 << 20 +// defaultMetadataStreamMaxSize is used when Proxy.MetadataStreamMaxSize is +// unset. +const defaultMetadataStreamMaxSize = 1 << 30 + // ErrMetadataTooLarge is returned when upstream metadata exceeds the configured limit. var ErrMetadataTooLarge = errors.New("metadata response exceeds size limit") @@ -215,14 +219,17 @@ type Proxy struct { Cooldown CooldownPolicy // ShortestCooldown is the shortest cooldown configured anywhere. It // bounds how long a rewrite filtered by cooldown is cached. - ShortestCooldown time.Duration - Denylist *denylist.Policy - CacheMetadata bool - MetadataTTL time.Duration - MetadataMaxSize int64 - GradleReadOnly bool - HostedEnabled bool - GradleMaxUploadSize int64 + ShortestCooldown time.Duration + Denylist *denylist.Policy + CacheMetadata bool + MetadataTTL time.Duration + MetadataMaxSize int64 + // MetadataStreamMaxSize caps a metadata document streamed from upstream + // into storage, which metadata_max_size doesn't cover. Default 1 GB. + MetadataStreamMaxSize int64 + GradleReadOnly bool + HostedEnabled bool + GradleMaxUploadSize int64 // NPMFullMetadata requests full npm packuments from upstream even when // cooldown is disabled, so served metadata carries publish times. NPMFullMetadata bool diff --git a/internal/handler/metadata_stored.go b/internal/handler/metadata_stored.go index b3bc4202..ffe524d9 100644 --- a/internal/handler/metadata_stored.go +++ b/internal/handler/metadata_stored.go @@ -198,9 +198,13 @@ func (p *Proxy) storeUpstreamMetadata(ctx context.Context, ecosystem, cacheKey, if lm := resp.Header.Get(headerLastModified); lm != "" { meta.lastModified, _ = http.ParseTime(lm) } + limit := p.MetadataStreamMaxSize + if limit <= 0 { + limit = defaultMetadataStreamMaxSize + } // Go decompresses the body, since no Accept-Encoding was set, so it is // stored uncompressed. - var body io.Reader = upstreamBodyReader{resp.Body} + var body io.Reader = &limitedBody{r: upstreamBodyReader{resp.Body}, left: limit} if validate != nil { staged, err := p.stageMetadata(ctx, ecosystem, cacheKey, body, validate) if err != nil { @@ -246,6 +250,21 @@ func (p *Proxy) stageMetadata(ctx context.Context, ecosystem, cacheKey string, b return &stagedReader{ReadCloser: staged, discard: discard}, nil } +// limitedBody fails with ErrMetadataTooLarge, as an upstream failure, once +// r has sent more than left bytes. +type limitedBody struct { + r io.Reader + left int64 +} + +func (l *limitedBody) Read(b []byte) (int, error) { + n, err := l.r.Read(b) + if l.left -= int64(n); l.left < 0 { + return 0, &upstreamBodyError{ErrMetadataTooLarge} + } + return n, err +} + // notStored returns a failure to store a document as errMetadataNotStored, // unless reading it from upstream failed. func notStored(what string, err error) error { diff --git a/internal/handler/npm_stream_harden_test.go b/internal/handler/npm_stream_harden_test.go index ca7143fe..0cebf0a7 100644 --- a/internal/handler/npm_stream_harden_test.go +++ b/internal/handler/npm_stream_harden_test.go @@ -181,3 +181,58 @@ func TestNPMStreamedPackumentClientRevalidates(t *testing.T) { t.Errorf("served %d versions, want 401", len(doc.Versions)) } } + +// TestNPMStreamedPackumentOverStreamLimit checks that a document over +// MetadataStreamMaxSize is neither stored nor served: the stored copy is +// served instead, with and without cooldown, and with nothing stored the +// request fails. +func TestNPMStreamedPackumentOverStreamLimit(t *testing.T) { + for _, withCooldown := range []bool{true, false} { + t.Run(map[bool]string{true: "cooldown", false: "no cooldown"}[withCooldown], func(t *testing.T) { + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, store, handler := streamProxy(t, u) + if !withCooldown { + proxy.Cooldown = nil + } + proxy.MetadataMaxSize = int64(len(body) / 4) + proxy.MetadataStreamMaxSize = int64(len(body) + 1000) + want := len(decodeStreamed(t, serve(handler, "/big", nil)).Versions) + before := bigMetadataFiles(store) + + u.replace(bigPackument(500), `"v2"`) + proxy.MetadataTTL = time.Nanosecond + proxy.SetMetadataRewriteCacheSize(64 << 20) + + if doc := decodeStreamed(t, serve(handler, "/big", nil)); len(doc.Versions) != want { + t.Errorf("served %d versions, want the stored %d", len(doc.Versions), want) + } + after := bigMetadataFiles(store) + if len(after) != len(before) { + t.Errorf("storage holds %d files, %d before", len(after), len(before)) + } + for path, b := range before { + if after[path] != b { + t.Errorf("%s was changed", path) + } + } + }) + } + t.Run("uncached", func(t *testing.T) { + body := bigPackument(400) + u := newStreamUpstream(t, body) + proxy, store, handler := streamProxy(t, u) + proxy.MetadataMaxSize = int64(len(body) / 4) + proxy.MetadataStreamMaxSize = int64(len(body) / 2) + + if rec := serve(handler, "/big", nil); rec.Code != http.StatusBadGateway { + t.Errorf("status %d, want 502", rec.Code) + } + if files := bigMetadataFiles(store); len(files) != 0 { + t.Errorf("left %d files in storage", len(files)) + } + if entry, _ := proxy.DB.GetMetadataCache("npm", "big"); entry != nil { + t.Error("left a metadata cache row") + } + }) +} diff --git a/internal/server/server.go b/internal/server/server.go index 2f0bd68b..caebb807 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -268,10 +268,7 @@ func (s *Server) serve(listener net.Listener) error { if err != nil { return fmt.Errorf("configuring scanners: %w", err) } - proxy.CacheMetadata = s.cfg.CacheMetadata - proxy.MetadataTTL = s.cfg.ParseMetadataTTL() - proxy.MetadataMaxSize = s.cfg.ParseMetadataMaxSize() - proxy.SetMetadataRewriteCacheSize(s.cfg.ParseMetadataRewriteCacheSize()) + configureMetadata(proxy, s.cfg) proxy.GradleReadOnly = s.cfg.Gradle.BuildCache.ReadOnly proxy.HostedEnabled = s.cfg.Hosted.Enabled proxy.HostedNPMScopes = s.cfg.Hosted.NPMScopes @@ -618,6 +615,15 @@ func (s *Server) newContainerHandler(proxy *handler.Proxy) *handler.ContainerHan return containerHandler } +// configureMetadata applies the metadata cache settings to proxy. +func configureMetadata(proxy *handler.Proxy, cfg *config.Config) { + proxy.CacheMetadata = cfg.CacheMetadata + proxy.MetadataTTL = cfg.ParseMetadataTTL() + proxy.MetadataMaxSize = cfg.ParseMetadataMaxSize() + proxy.MetadataStreamMaxSize = cfg.ParseMetadataStreamMaxSize() + proxy.SetMetadataRewriteCacheSize(cfg.ParseMetadataRewriteCacheSize()) +} + func configureScanning(proxy *handler.Proxy, cfg config.ScanningConfig, baseURL string, logger *slog.Logger) (*scanner.Group, error) { scanGroup, err := scanner.NewGroup(cfg, logger) if err != nil {