Repository navigation
Conversation
|
I tried this with clients that send back the validators they were given (
With cooldown on I noticed a cost, though. npm still gets its 304s, a big saving since full packuments averaged about 330 KB here. But every npm metadata request got about 20% more expensive, conditional or not: 4.5 to 4.8 ms of CPU against 3.8 to 4.0 on Could the ETag come from the inputs instead? With cooldown on, the output is fixed by the upstream document's digest, the versions left after cooldown and denylist filtering (which also settles the dist-tags), and the startup config. Hashing those would stay exact without touching the full body. Happy to rerun the numbers if you give it a try. |
|
Thanks for measuring this. The ETag with cooldown on now comes from the inputs, as you suggested. It's the SHA-256 of the upstream document's digest, the proxy URL, the package name, the versions kept after cooldown and denylist filtering, and a per-process nonce that stands for the startup configuration. The digest isn't hashed again per request. It's the one the metadata cache already recorded when it stored the document, and it's passed along with the body. When there's no digest (metadata caching off), it falls back to hashing the body. With cooldown off nothing changes: the body hash is still computed once per cached rewrite. In a local benchmark on a 1.4 MB, 300-version packument, the ETag went from about 340 µs (body hash) to about 19 µs. I'd be glad if you could rerun your numbers. |
|
Nice, that fixed it! With cooldown on, npm metadata now costs about the same CPU as
With cooldown off nothing changed, as you said. In the later rounds, 10 of each tester's 15,380 revalidations came back 200 instead of 304, the same with both versions of the PR. I think that's a version crossing the cooldown boundary mid-test, so it looks right too. Thanks for the quick turnaround! |
andrew
left a comment
There was a problem hiding this comment.
The Last-Modified calculation can return a false 304 after refreshing cached metadata. I reproduced this through both the npm and Composer HTTP handlers.
| return time.Time{} | ||
| } | ||
| if upstream.Before(processStartedAt) { | ||
| return processStartedAt |
There was a problem hiding this comment.
Clamping every upstream date before startup to processStartedAt gives different documents the same Last-Modified. For example, a restart can serve an older persisted cache entry that is still within its TTL, then fetch a newer upstream document whose modification date also predates the restart. Both responses receive the startup date, so a client sending the first response's If-Modified-Since gets 304 even though the body changed. A handler-level reproduction for npm and Composer produced a changed body and ETag but HTTP 304. Track modification time per representation, or omit the date when freshness cannot be established, and add regression coverage for refreshing an older cache entry after restart.
There was a problem hiding this comment.
Thanks, you're right: clamping to the process start collapsed different documents onto one date. Fixed in 8ac4ce2.
Last-Modified no longer comes from upstream at all. It's the time this process first made the rewrite, kept with the rewrite cache entry. Every new rewrite of a package gets a date strictly later than the last one it got, including within the same second and after its entry was evicted, so no two representations share a date. With the rewrite cache off there's no Last-Modified, since nothing records when the rewrite was made. Cooldown still sends none.
Regression coverage is TestRewrittenMetadataLastModifiedFollowsRefresh, through the npm and Composer handlers, with TTL 1h (row expired) and 0. Upstream changes to a newer document that it also dates before the process started, and If-Modified-Since from the first response must get 200 with the new body. It failed with your 304 before the fix. TestRewriteCacheLastModified covers same-second rewrites and eviction with a fake clock.
The cost is one unneeded 200 per package after a restart or eviction, never a false 304. I also rebased onto main to resolve the conflict with #418.
7518747 to
8ac4ce2
Compare
andrew
left a comment
There was a problem hiding this comment.
The startup-date clamping issue is fixed, but two false-304 cases remain in the revised Last-Modified handling. Both were reproduced through the npm and Composer HTTP handlers.
| e := el.Value.(*rewriteEntry) | ||
| c.mu.Unlock() | ||
| return out, nil | ||
| return e.out, e.metadataValidators, nil |
There was a problem hiding this comment.
Reusing a cached rewrite also reuses its old Last-Modified, even if another representation was served in between. With upstream documents A, then B, then A again, the final response carries A's original date. A client holding B and sending B's If-Modified-Since gets 304 despite the changed body and ETag. I reproduced this through both handlers with the clock advancing ten seconds between changes. Track when the current representation changes, including transitions to a previously cached body, or omit the date when that cannot be established. Add an A-to-B-to-A handler regression test.
There was a problem hiding this comment.
Confirmed and fixed in ab0f057. A rewrite was dated when it was made, and that date travelled with its cache entry, so a cached A could come back older than B.
The cache now records which rewrite each package is serving and since when. Last-Modified is the second the package switched to the rewrite, so A coming back after B is dated at the switch, later than anything B was dated. TestRewrittenMetadataLastModifiedReturningDocument covers A → B → A through both handlers, with and without a TTL, and the clock moves ten seconds between changes. It got 304 with B's date before the fix.
| pkg := key[:strings.LastIndexByte(key, 0)] | ||
| lastModified := now | ||
| if issued, ok := c.issued[pkg]; ok && !issued.Before(now) { | ||
| lastModified = issued.Add(time.Second) |
There was a problem hiding this comment.
Incrementing the timestamp for same-second rewrites issues Last-Modified dates ahead of the clock. That history is process-local, so after a restart a changed document can receive a date earlier than one the client already holds, and notModified returns a false 304. I reproduced this through both handlers by serving two different documents at the same clock time, replacing the rewrite cache with a fresh instance to model restart, then serving changed metadata with the second response's If-Modified-Since. Avoid manufactured future dates and add restart coverage; return the body when the timestamp cannot safely validate the client's representation.
There was a problem hiding this comment.
Confirmed and fixed in ab0f057. The +1s bump is gone, and no date is ever ahead of the clock.
- When a package switches rewrites in a second that already dated another of its rewrites, the new one goes out with no Last-Modified until the next second. The client then has no date to send and gets the full body.
- A new rewrite cache dates nothing in the second it starts, since the previous process may have dated a rewrite in that second.
- Dropping a package's record on eviction follows the same rule. If the record was dated in the current second, new records wait for the next second.
TestRewrittenMetadataLastModifiedAcrossRestart follows your reproduction through both handlers. It serves two documents at the same clock time, replaces the rewrite cache to model the restart, then sends the latest date the client got with changed metadata. It checks that the response is a 200, that no date was ahead of the clock, and that dating resumes the next second. TestRewrittenMetadataLastModifiedSameSecond covers the undated window. Both failed before the change.
The guarantee is per process: replicas behind a load balancer keep separate records. The branch is also rebased onto main after #426.
8ac4ce2 to
ab0f057
Compare
andrew
left a comment
There was a problem hiding this comment.
The previous single-process findings are fixed, but process-local dates still allow false 304 responses across replicas. The Helm chart supports multiple replicas, and sharing database and storage does not share the rewrite cache's serving map.
I reproduced this through both npm and Composer handlers with two proxy instances sharing database and storage, TTL 0, and synchronized clocks one second after startup:
- Instance A serves the original document with
Last-Modified: Sat, 10 Oct 2026 12:00:01 GMT. - Upstream changes the document in that second.
- A conditional request to instance B carrying that date returns 304 with a different ETag. The client retains the old document.
Please make date validation safe across replicas, or omit it when that cannot be guaranteed. A small safe fallback in rewrite_cache.go is:
func (c *rewriteCache) validators(e *rewriteEntry) metadataValidators {
return metadataValidators{etag: e.etag}
}The existing serveRewrittenMetadata and notModified paths already omit Last-Modified and decline If-Modified-Since validation when the date is zero. ETag revalidation still works. This would defer the Composer date-based bandwidth saving, so another option is an explicit single-instance opt-in for date validation, disabled by default. Merely omitting the response header while still evaluating the internal date would not fix it.
I checked the ETag-only hint in an isolated copy: the two-instance regression returns 200 with changed metadata, and the existing ETag revalidation tests still pass. Please add a two-instance handler regression that changes upstream between requests in the same second and verifies the old date cannot suppress the new body, including dates held by clients from before the fix.
ab0f057 to
171a60e
Compare
|
Confirmed, thanks for the two-instance reproduction. Fixed in f68b87e and 171a60e by moving the dates into the shared database, so date validation stays on and is safe across replicas.
Regression tests, through the npm and Composer handlers:
I checked that each rule is needed: removing the same-second check fails the two-replica test, removing the start-second check fails the pre-fix test, and serving dates ahead of the clock fails the skew test. I also ran two proxy binaries against one Postgres. Both served the same Last-Modified from one row, and B answered 304 to A's date. Two limits:
The branch is rebased onto main. |
andrew
left a comment
There was a problem hiding this comment.
Please add upgrade regression coverage for migration 011 on both SQLite and PostgreSQL, starting with the previous schema and representative existing npm/Composer metadata.
TestMetadataDatesTableCreatedByMigration currently creates an empty SQLite database, drops metadata_dates, removes the migration record, and checks only that the table is recreated. It does not verify that existing cached metadata survives the upgrade or that the npm and Composer handlers serve correct validators and conditional responses afterward.
Seed cached documents before applying the migration, verify the existing rows and stored bytes are preserved, then exercise the handlers after upgrade, including a changed document and conditional requests from two replicas.
- Rewritten metadata went out with only Content-Type, so pnpm, npm and Composer could never revalidate and downloaded every document in full - ETag is the SHA-256 of the rewritten body, computed once per rewrite and kept with the rewrite cache entry; exact under cooldown as well - Last-Modified is the upstream Last-Modified, never earlier than the process start, since a restart can change the rewrite; omitted with cooldown on - Answer If-None-Match, and If-Modified-Since when there is no If-None-Match, with 304 on the rewrite fast path and on the fetch path
- With cooldown on every rewrite runs, and hashing each rewritten body for its ETag made every npm metadata request about 20% more expensive - Hash what fixes the output instead: the upstream digest, proxy URL, package name, the versions cooldown and the denylist kept, and a per-process nonce standing for the startup configuration - Carry the stored document's digest and upstream Last-Modified with the metadata lookup result, so neither is read from a second cache row lookup that a concurrent refresh could have changed - Fall back to hashing the body when the digest is unknown, and keep the body hash with cooldown off, where it is computed once per cached rewrite
Last-Modified was upstream's date clamped to the process start, so different documents could share a date: after a restart, a cached document and the newer one a refresh fetched were both dated at startup, and a client holding the first got 304 for the second. - Date each rewrite by when the rewrite cache first made it, and keep that date while the entry stays cached - Give every new rewrite of a package a date later than the last one it got, even within the same second or after its entry was evicted, so no two representations share a date; dates are remembered only until the clock passes them - Send no Last-Modified when the rewrite cache is off, since nothing records when an uncached rewrite was made; cooldown still sends none - Drop the upstream Last-Modified from metadata lookups, which nothing uses now
Two false 304s remained. A rewrite kept the date it was made, so when upstream went from A to B and back to A, A's cached rewrite came back with a date earlier than B's and a client holding B got 304. And a second rewrite in the same second was dated a second ahead of the clock, a date only that process remembered: after a restart a changed document could be dated earlier than one the client held. - Record which rewrite each package is serving and since when, and date a rewrite by the second its package switched to it, including back to one still cached - Never date ahead of the clock: a rewrite switched to in a second that already dated another for the package goes undated until the next second - Date nothing in the second the cache starts, the last second the previous process may have dated a rewrite in - Drop a package's record when its current rewrite is evicted, so records stay bounded by the cache; forgetting one dated in the current second holds back dates for new packages until the next second
Replicas sharing a database need one place to agree on the Last-Modified of rewritten metadata, which process memory cannot give them. - Add metadata_dates, keyed by ecosystem, proxy URL and package, holding the ETag being dated and its date in Unix seconds, so no zone is involved - Insert a package's first date only if no replica recorded one first - Swap a date only if it is still the one read, so of two replicas switching a package at once only one succeeds - Create it in both schemas and in migration 011
Dates were kept in each process's rewrite cache, so replicas sharing a database and storage dated independently: one dated a document, upstream changed in that second, and another gave the new document the same date, so a client holding it got 304 and kept the old document. - Date from metadata_dates instead of process memory: a package's date is the second it started serving its current representation, recorded once for every replica - Switch a package to another representation only in a second later than its recorded date, and serve it undated until then - Date nothing in the second the process started, so dates clients hold from before this change, or from the process it replaced, stay older - Serve no date ahead of the local clock, recorded by a replica whose clock is ahead - Date rewrites without the rewrite cache too; the cache keeps only ETags - Add two-replica, pre-fix date and clock skew regression tests
171a60e to
054c45c
Compare
- Start from the schema before migration 011 on SQLite and PostgreSQL, with npm and Composer metadata cached, and check the rows and stored bytes survive the migration unchanged - After the upgrade, check two replicas serve the cached document with matching ETag and Last-Modified and answer conditional requests with 304 - Change the upstream document and check neither replica answers the old validators with 304, and both agree on the new ones - Give the PostgreSQL handler test a database of its own, so it shares no tables with the database package tests on the same server
|
Re review: added upgrade regression coverage for migration 011 on SQLite and PostgreSQL in 9f42425:
The PostgreSQL cases run when |
Problem
Rewritten npm and Composer metadata goes out with only a
Content-Typeheader. Without a validator, clients can't revalidate their copy, so every metadata request downloads the whole document again. Withnpm_full_metadataon, that can be megabytes per package.Both client families already revalidate when they can:
If-None-Matchwith the storedETag. pnpm's resolver also caches a packument'setag, and treats a registry without one as unable to answer 304.If-Modified-Sincewith the storedLast-Modifiedforp2metadata, and never sendsIf-None-Match.Changes
ETag (
rewrite_cache.go)Last-Modified
Last-Modified, so the client fetches the full body next time instead of risking a false 304.Conditional requests
304that carries the validators, on both the rewrite fast path and the fetch path.If-None-Matchdecides when present (weak comparison, so ETags weakened by a gzip-ing reverse proxy still match).If-Modified-Sinceis used only without it, as RFC 9110 specifies.Unchanged
Verification with real clients
Built proxy,
cache_metadataandnpm_full_metadataon, two consecutive runs with a shared client cache:composer update(Composer 2.8, monolog + symfony/console)npm install --package-lock-only --prefer-onlinepnpm 12.9.1 stores the ETag in its metadata cache. In this setup it resolved from its store on the second run without contacting the registry, so it sent no conditional request to observe.
Tests
metadata_validators_test.go, for both npm and Composer:If-None-Matchvalues all give an empty 304 that carries the ETag. A new upstream document gives a new ETag.If-Modified-Sincegives 304 at or after it and 200 before it. A mismatchedIf-None-Matchoverrides a matchingIf-Modified-Since.If-Modified-Sincefrom the first response gets 200 with the new body and a laterLast-Modified.If-Modified-Sincewith B's date gets 200 with A.Last-Modified, andIf-Modified-Sincewith the first one's date gets 200. It is dated the next second.If-Modified-Sincewith the latest date the client got gives 200, no date is ever ahead of the clock, and a date is given from the next second on.Last-Modified. The ETag changes when a version leaves cooldown and comes from the rewrite's inputs when the digest is known.Last-Modified.go test -race ./...andgolangci-lint run ./...(v2.13.1) pass.Not in this PR
ProxyCached(used by APK and other ecosystems) still reads the whole stored document before checking conditional headers, and looks up the metadata row twice per hit. It also letsIf-Modified-Sinceproduce a 304 even when a presentIf-None-Matchdoesn't match. A follow-up could move it onto the samenotModifiedcheck.Rebased onto main after #426 merged. Routed packages' cache keys carry through the metadata lookups and rewrite cache keys.