[simonchrz]

Closes #525.

With cache_metadata and the rewrite cache on, an npm packument is streamed from upstream into storage and rewritten from there in two passes, as the issue describes:

• npm.ScanPackument reads the stored document once and keeps the time map, dist-tags and version names. npm.RewriteStream reads it again through internal/jsonstream, copying each kept version with its tarball URL rewritten and dropping the rest. Its output matches npm.Rewrite except for whitespace inside a filtered time map; a test compares the two over documents with the members in either order, duplicates and values copied verbatim.
• storedMetadataDocument streams the upstream body into storage without ReadMetadata, so metadata_max_size doesn't apply. It shares fetches, leases, 304 revalidation and the stale fallback with the in-memory lookup. With cooldown, a fetched packument is staged under its own path and checked before it replaces the cached copy, so an abbreviated one never overwrites a full one. A failure to store falls back to the in-memory path.
• Both passes check the stored bytes against the row's digest, so a document replaced between them fails instead of being rewritten from mixed bytes.
• The rewrite is kept gzip-compressed only, and a client not accepting gzip gets it decompressed as it is sent.
• An abbreviated packument where the full one is required, a document without a versions object, and any rewrite error fall back to the in-memory rewrite, which then reads the stored copy.

Request tests through /npm/... with cooldown on cover a packument larger than metadata_max_size (the in-memory path returns 502 for it), time after versions and latest inside cooldown, the stale fallback, 404, a 304 that keeps the stored document, a document changed in storage, an abbreviated packument from upstream keeping the full copy, a failing store, version and dist-tag requests, and the tarball cooldown and URL lookup.

Against registry.npmjs.org with cooldown.default: 3d, the first request for renovate (73 MB) peaked at 60 MB RSS, against 284 MB before, at about the same time (1.07s vs 0.95s).

/<package>/<version> and dist-tags resolve the same way (npm.VersionStream), and the tarball download's cooldown, integrity and attestations lookups read only the one version entry (npm.LookupVersion). For renovate, a version request plus a tarball download peaked at 49 MB, against 283 MB.

[andrew]

Two availability regressions need fixing before merge. I reproduced both through npm handler requests with cooldown and metadata caching enabled, comparing the streaming path with the existing in-memory path.

1. storeUpstreamMetadata in internal/handler/metadata_stored.go replaces the cached document before serveStreamedPackument checks whether full metadata is required. After seeding a full packument and expiring its TTL, an upstream response containing an abbreviated packument overwrites the usable full copy. The streaming rewrite rejects it, and the in-memory fallback can no longer serve the old full document: the request returns 502, whereas the existing path rejects the abbreviated response before caching and serves the old copy with 200. Please validate a staged copy before replacing the existing cache, and add a regression test that verifies both the response and the retained cache contents.

2. serveStreamedPackument in internal/handler/npm.go treats storage write failures as upstream failures. With a healthy upstream, an uncached packument below metadata_max_size, and Storage.Store returning a disk-full error, the streaming path returns 502. The existing in-memory path still returns the filtered document with 200 because caching is best effort. Please fall back to the bounded in-memory path when storage fails, and cover this with a handler regression test.

[simonchrz]

Both fixed in 8c9e87f.

1. With cooldown needing the full packument, a fetched document is now stored under a staging path of its own and checked there before it replaces the cached copy. An abbreviated one is discarded and the full copy is served, as when upstream fails. TestNPMStreamedPackumentKeepsFullCopy checks the response, that the stored blob is still the full one, and that no staged copy is left.
2. A failure to store returns errMetadataNotStored, and the request takes the in-memory path, which serves the document as before. TestNPMStreamedPackumentStorageFails covers it.

The same commit also streams the two npm paths the description listed as not covered: /<package>/<version> and dist-tags resolve through two passes over the stored packument, and the tarball download's cooldown, integrity and attestations lookups read only the one version entry. With renovate, a version request plus a tarball download peaked at 49 MB RSS, against 283 MB before.

Main is merged in, so #638 no longer shows here.

[andrew]

Two availability failures and a regression-test gap remain:

1. internal/handler/metadata_stored.go:125 skips stale fallback on ErrUpstreamNotFound. With an expired cached packument and an upstream 404, a handler request returns 404 through streaming while the existing in-memory path serves the cached document with 200. Preserve the existing stale fallback and add a request-level test with a cached document.

2. internal/handler/metadata_stored.go:157 accepts a 304 without checking that the stored blob remains usable. I reproduced this by caching a packument larger than metadata_max_size, deleting its blob while retaining the row, expiring the TTL, and clearing the rewrite cache. The next handler request returns 502 against a healthy upstream: the 304 refreshes the row, then recovery uses the bounded in-memory path. Retry through streaming without validators when the stored blob is missing, and cover recovery for a document over the size limit.

3. internal/handler/npm_stream_test.go:265 does not exercise the abbreviated replacement. newStreamUpstream returns 304 whenever If-None-Match is "v1", before reading the replacement body, so this test passes without exercising staging or rejection. Make the replacement return 200, assert that it was sent, and retain the response and stored-content checks.

[simonchrz]

Fixed in 760623e.

1. An upstream 404 after the TTL now falls back to the stored copy, as the in-memory path does. TestNPMStreamedPackumentStaleOn404 serves a cached packument with upstream returning 404, through both the streamed and the in-memory path.
2. On a 304 the stored blob is checked first. If it's gone, the document is fetched again without validators and streamed into storage. TestNPMStreamedPackumentBlobMissing caches a packument over metadata_max_size, deletes its blob, expires the TTL and clears the rewrite cache, then checks for a 200 with all versions and that the blob is stored again.
3. The test upstream now takes an ETag per body, so the abbreviated replacement comes back in a 200. TestNPMStreamedPackumentKeepsFullCopy asserts it was sent, and keeps the response, stored-content and staged-copy checks.

The first two tests fail on the previous commit.

[andrew]

Missing-blob recovery still fails while the metadata cache row is within its TTL. In internal/handler/metadata_stored.go:99, storedMetadataEntryState treats the row as fresh without checking whether its blob exists. The streaming read then fails, and recovery falls back to the bounded in-memory path.

I reproduced this through an npm handler request: cache a packument above metadata_max_size, delete its blob while retaining the fresh row, clear the rewrite cache, then request the package again. It returns 502 against a healthy upstream. The same test with an expired row returns 200, so the new 304 recovery handles that case. Please recover missing blobs through streaming regardless of TTL, and extend TestNPMStreamedPackumentBlobMissing to cover a still-fresh row and verify that its blob is restored.

[simonchrz]

Fixed in 11ff4cf.

A read that finds the stored copy missing now fetches the packument again, ignoring that row whether it's within its TTL or not, and reads it once more. It checks on the failed read rather than checking the blob on every request, so a fresh hit doesn't cost an extra stat or HEAD.

TestNPMStreamedPackumentBlobMissing now runs with a fresh and an expired row and checks that the blob is stored again. TestNPMStreamedVersionBlobMissing covers the same for a version request and a tarball download. Both fail on the previous commit.

[simonchrz]

More hardening in f0c96e8, from new tests:

1. Upstream cuts a document off (connection closed before its Content-Length). Streamed, the read error counted as a failure to store, so a packument over metadata_max_size got a 502 instead of the stored copy. This also happens on main, in memory: ReadMetadataSized took the transport's io.ErrUnexpectedEOF for the end of a body shorter than its size hint, so the half document was served with a 200 and cached (without cooldown), or failed the filter with a 502 (with cooldown). Upstream bodies are now read through a reader that marks their read errors, and both paths treat a cut-off body as an upstream failure. TestNPMStreamedPackumentTruncated and TestNPMStreamedPackumentTruncatedUncached cover both paths with and without cooldown, and TestReadMetadataSizedTruncatedUpstream covers the reader.
2. Stored bytes don't match the row's digest within the TTL: fetched again like a missing blob (TestNPMStreamedPackumentBlobCorrupt).

Also new: concurrent requests sharing one fetch, client revalidation with If-None-Match, and 404/502 with nothing cached.

While writing the concurrency test I found that cooldown.Config parses itself on first use without locking (parse() sets parsed before the durations), so concurrent first calls can see cooldown as off. The server isn't affected, since cooldown.NewPolicy parses at startup, but the stream tests used a bare Config and now build it through NewPolicy.

[andrew]

Three things before this can merge.

1. The streamed path has no size limit. storeUpstreamMetadata in internal/handler/metadata_stored.go copies the upstream body into storage uncapped, twice with cooldown (staged, then final). ScanPackument, LookupVersion and jsonstream decode each top-level member whole through json.Decoder, so one oversized member such as readme is held in memory. metadata_max_size was the protection against a misbehaving upstream, and the docs change says it no longer applies here. Please cap the upstream body on this path and add a request-level test with a body over the cap. In the same paragraph of docs/configuration.md, the sentence saying /<package>/<version> requests still read the packument into memory is out of date, since 8c9e87f streams them.

2. The streamProxy helper in internal/handler/npm_stream_test.go switched to cooldown.NewPolicy to avoid the race in cooldown.Config.parse, while every other handler test assigns a cooldown.Config directly. That race is a cooldown bug, tracked in git-pkgs/cooldown#15, and gets fixed there. Once a cooldown release with the fix is bumped here, please revert the helper and its comment. This PR waits on that release.

3. Please move the truncated-body change to fetchUpstreamMetadata and ReadMetadataSized, with TestReadMetadataSizedTruncatedUpstream and the in-memory cases of the truncation tests, into its own PR. It changes behaviour for every ecosystem, so it needs its own review and its own revert. upstreamBodyReader stays here for storeUpstreamMetadata. Please keep further rounds to what the review asks for: the version and tarball streaming in 8c9e87f and this change both arrived as additions in fix-up rounds.

[simonchrz]

1. Fixed in b08cebf. The streamed body is now capped by a new metadata_stream_max_size, 1 GB by default (PROXY_METADATA_STREAM_MAX_SIZE). The cap wraps the upstream body before staging, so it covers both copies with cooldown. A document over it fails as an upstream failure: nothing is stored, and the stored copy is served if there is one, otherwise 502. TestNPMStreamedPackumentOverStreamLimit covers this through the handler with and without cooldown, and with nothing stored. The docs paragraph now says version requests and tarball downloads read from the stored copy, and documents the cap. To keep serve under the maintidx limit, the metadata settings moved into a small configureMetadata helper.
2. Understood. I'll revert the helper and its comment once a cooldown release with the fix for Config.parse races on concurrent first use cooldown#15 is bumped here.
3. Moved to Fail metadata reads when upstream cuts a document off #649 in 6dbda21. upstreamBodyReader now lives in upstream_body.go, which Fail metadata reads when upstream cuts a document off #649 adds with the same content, so the two merge cleanly in either order.

I also merged main (98ff4fd) for a conflict in cmd/proxy/main.go.