Skip to content

fix(handler): let the ProxyCached path request a per-call Accept-Encoding - #324

Open
pinguinfuss wants to merge 11 commits into
git-pkgs:mainfrom
pinguinfuss:issue-305-transfer-compression
Open

fix(handler): let the ProxyCached path request a per-call Accept-Encoding#324
pinguinfuss wants to merge 11 commits into
git-pkgs:mainfrom
pinguinfuss:issue-305-transfer-compression

Conversation

@pinguinfuss

@pinguinfuss pinguinfuss commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Since #304 every ProxyCached caller asks upstream for identity. Right for signed indexes, but it drops transfer compression for everyone — the Homebrew API alone refetches ~33 MB of formula.jws.json that would be ~5 MB gzipped (#305).

The internal verbatim bool becomes an acceptEncoding string ("" = leave unset, identity, gzip). ProxyCached still sends identity, so nothing changes for the other callers. Homebrew asks for gzip on the API paths — brew fetches them with curl --compressed and decodes the header itself — except analytics/, which brew fetches without --compressed. Setting the header ourselves keeps Go from decompressing, so the compressed bytes and Content-Encoding are cached and replayed as they came in.

Three things had to move with that, all only harmful once a caller can ask for gzip:

  • fetchOrCacheMetadata returns the encoding of the body it hands back and the writer uses it, instead of re-reading the cache row. Otherwise a failed Storage.Store served gzip bytes as application/json with no header. writeMetadataCachedResponse keeps its signature (helm/maven untouched) and delegates to a …WithEncoding sibling.
  • cacheMetadataBlob no longer swallows the row-write error: it deletes the blob it just stored, so a later hit can't serve a gzip blob under the old row.
  • The stale fallback re-reads the row before serving, so a request that lost the upstream race to one storing a gzip blob doesn't label it with the identity row.

Not switched globally on purpose: apt, apk, dnf, hex, R, bundler and Pkg.jl don't decode unsolicited Content-Encoding: gzip, and their upstreams send the same bytes either way.

Tests in proxy_cached_encoding_test.go and homebrew_test.go; #304's identity tests are unchanged. The ETag / Last-Modified validators still come from the cache row — that predates #304.

Fixes #305

git-pkgs#304 made the ProxyCached path request Accept-Encoding: identity so the
metadata cache stores upstream bytes verbatim. That is required for the
signed / hash-pinned index ecosystems, but conda's repodata.json is large
plain JSON: linux-64 repodata.json is ~441 MB uncompressed (over the
metadata_max_size cap, so it 502s today) versus ~34 MB gzip.

Replace the ProxyCached path's verbatim bool with an explicit
acceptEncoding string ('' = leave unset / transparent, 'identity', or
'gzip'), reusing git-pkgs#304's existing store-and-replay of Content-Encoding
unchanged. ProxyCached keeps its exported signature and continues to send
identity, so the nine other ecosystems and helm/maven are untouched; only
conda's repodata.json / current_repodata.json now request gzip. Setting
Accept-Encoding explicitly disables Go's transparent decompression, so the
compressed bytes and the Content-Encoding: gzip header are cached and
replayed exactly as identity bytes are. conda, mamba and pixi solicit and
decode gzip on .json URLs; repodata.json.bz2 stays identity.

Fixes git-pkgs#305
The adversarial review of the conda gzip route found a reachable
regression: writeMetadataCachedResponse took Content-Encoding from a
fresh cache-row read while cacheMetadataBlob skips the row write when
Storage.Store fails. Under identity that was benign (the body was plain
anyway), but on the new gzip route a disk-full or object-store outage
served raw gzip bytes as Content-Type: application/json with no
Content-Encoding and HTTP 200 -- conda, mamba and pixi fail to parse
them, with no HTTP signal and only a Warn log, on every request until a
cache write succeeds.

fetchOrCacheMetadata now returns the encoding of the body it hands back
(the upstream value on a fetch, the stored row's value on a TTL hit or
stale fallback) and proxyCachedWithEncoding passes it to
writeMetadataCachedResponse, so the header always describes the bytes
actually written. cachedMeta drops its now-unused content_encoding
field. helm and maven pass "" -- both fetch transparently, so their
stored encoding was always empty and behaviour is unchanged.

Also fixes a vacuous assertion in the new conda test: the upstream
request counter incremented behind the availability gate, so the
cached-replay block could never observe a refetch.
… guard

Follow-ups from the adversarial review of the git-pkgs#305 branch, limited to
code this branch introduced:

- proxyMetadataStream is only ever reached with an explicit
  Accept-Encoding (ProxyCached passes identity, conda passes gzip or
  identity), so the guard around the header set was unreachable; replace
  it with the plain one-token substitution of the former literal, which
  is the smallest change from main.
- The stale-fallback return of fetchOrCacheMetadata (encoding taken from
  the cache row) was the one git-pkgs#305 return site no test pinned: replacing
  it with an empty encoding survived the whole suite. Add a conda test
  that expires the entry, fails the upstream, and asserts the stored
  gzip blob is served with Content-Encoding: gzip.

Not changed, by scope: cacheMetadataBlob still discards the
UpsertMetadataCache error (pre-existing on main). If Storage.Store
succeeds and the row write fails, a later stale fallback or TTL hit can
serve the gzip blob with the row's stale encoding; that needs a DB write
failure plus a second event and is tracked separately.
The third adversarial review classified deleting cachedMeta.contentEncoding
and its lookupCachedMeta populate as elective: neither line was created by
this branch nor forced by the fix (writeMetadataCachedResponse now reads
the encoding from its parameter and ignores the row value). Under the rule
that pre-existing code this branch did not have to touch stays untouched,
restore both as they are on main. No behaviour change.

Residuals the review documented, unchanged by scope (both share one root
cause: the encoding lives in the cache row and the bytes in the blob, and
neither is written or read atomically):

- cacheMetadataBlob discards the UpsertMetadataCache error, so after a
  successful gzip Store and a failed row write a later stale fallback or
  TTL hit can serve the gzip blob with the row's stale encoding.
- During the one-time identity->gzip rollout, a request that read a
  pre-branch identity row, lost the upstream race to a request that stored
  the gzip blob, and then failed upstream serves the gzip bytes with no
  Content-Encoding for that one response; later requests self-heal.
- helm and maven now pass an empty encoding; on main a spec-violating
  upstream that answered a transparent gzip request with an encoding Go
  does not decode (e.g. br) would have had that header replayed from the
  row. Degenerate; documented rather than changed.
Threading acceptEncoding through CondaHandler.proxyCached changed the
form of two pieces of original code the fix did not need to touch: the
repodata.json.bz2 route (method value rewritten as a closure) and
proxyCached itself (new parameter, new call). Restore both exactly as on
main; ProxyCached still sends identity, so the .bz2 route is unchanged in
behaviour. handleRepodata's non-cooldown branch now derives the cache key
inline and calls proxyCachedWithEncoding with gzip directly, so the only
original conda.go line that changes is that one call.
…main

Adding a contentEncoding parameter to writeMetadataCachedResponse changed
a signature that predates git-pkgs#304 and dragged its two pre-git-pkgs#304 callers
(helm.go, maven.go) into the diff, even though git-pkgs#304 only ever added the
cm.contentEncoding block inside the function body.

Restore writeMetadataCachedResponse's doc and signature exactly as on
main and make it a delegate that passes an empty encoding to a new
unexported writeMetadataCachedResponseWithEncoding, which carries the
original body with git-pkgs#304's block reading the parameter instead of the
cache row. proxyCachedWithEncoding calls the sibling with the encoding
returned alongside the body. helm.go and maven.go drop out of the diff;
their behaviour is unchanged (both fetch transparently, so their stored
encoding was always empty). Same split pattern as ProxyCached ->
proxyCachedWithEncoding.
The conda call site in handleRepodata predates git-pkgs#304 and git-pkgs#304 never
touched it, so under the rule that this PR only corrects code and
behaviour git-pkgs#304 introduced it does not belong here. Restore conda.go and
conda_test.go as on main; the conda change continues on a stacked branch
against its own issue.

Replace the conda-route tests with tests that exercise
proxyCachedWithEncoding directly, so this PR still pins its own plumbing:
gzip is requested and the compressed bytes plus Content-Encoding are
cached and replayed (cached and streaming paths), the header survives a
metadata cache write failure, and the stale fallback keeps the stored
encoding.
Resolves the conflict in proxyMetadataStream: keep the acceptEncoding
parameter from this branch and the r.Method request from main (git-pkgs#267).
Homebrew (git-pkgs#254) routes every API path through ProxyCached and so, since
git-pkgs#304, fetches formula.jws.json (~33 MB plain, ~5 MB gzip) uncompressed on
every refresh -- the case that motivated git-pkgs#305.

Request gzip for the JSON API via proxyCachedWithEncoding: brew fetches
every API download with curl --compressed and decodes Content-Encoding
itself, so the compressed bytes and header are cached and served as-is
and both hops stay compressed. The analytics endpoints are the one brew
consumer fetched without --compressed; they stay on identity.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Response conditional/validator handling can become inconsistent with the served body when cache writes fail, which can lead to incorrect headers or 304 responses.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors the cached-metadata fetch path to support a per-call upstream Accept-Encoding policy, enabling callers to opt into requesting gzip (and caching/replaying the encoded bytes) while keeping the default ProxyCached behavior as Accept-Encoding: identity for byte-for-byte fidelity of signed or hash-pinned metadata.

Changes:

  • Replace the internal verbatim bool with acceptEncoding string and thread it through upstream fetch + streaming paths to control Go’s transparent decompression behavior.
  • Return the body’s effective Content-Encoding from fetchOrCacheMetadata and use it when writing responses to avoid mislabeling bytes when cache writes fail.
  • Add proxyCachedWithEncoding and introduce focused tests covering gzip caching/replay, streaming mode, store-failure behavior, and stale-cache fallback.
File summaries
File Description
internal/handler/handler.go Adds per-call upstream Accept-Encoding handling and plumbs Content-Encoding through fetch/cache/write paths.
internal/handler/proxy_cached_encoding_test.go New tests validating gzip behavior across cached, streaming, store-failure, and stale-fallback scenarios.
Review details

Suppressed comments (1)

internal/handler/handler.go:1003

  • writeMetadataCachedResponseWithEncoding always looks up ETag/Last-Modified from the DB cache entry, even when serving a freshly-fetched body after a cache write failure. If Storage.Store or the DB upsert fails but an older cache entry exists, this can emit validators that don’t match the bytes being served (and can incorrectly return 304 to If-None-Match/If-Modified-Since). Consider having fetchOrCacheMetadata return the validators for the body it returns (etag/lastModified/stale) and/or a flag indicating whether the cache row was successfully updated, and only apply conditional logic based on those values when they correspond to the served body.
	cm := p.lookupCachedMeta(ecosystem, cacheKey)

	if cm.etag != "" {
		w.Header().Set(headerETag, cm.etag)
	}
	if !cm.lastModified.IsZero() {
		w.Header().Set(headerLastModified, cm.lastModified.UTC().Format(http.TimeFormat))
	}
	if ifNoneMatchHits(r.Header.Get("If-None-Match"), cm.etag) {
		w.WriteHeader(http.StatusNotModified)
		return
	}
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/handler/handler.go
…an empty value

fetchUpstreamMetadata treats an empty acceptEncoding as 'do not set the
header'; proxyMetadataStream set it unconditionally, which would send an
empty Accept-Encoding line if a caller ever passed . Guard it the same
way so both paths agree. No caller passes  today.
…t bytes

Two ways the cache row could stop describing the stored blob once a
caller requests gzip, both raised by the review of git-pkgs#324:

- cacheMetadataBlob stored the blob and then discarded the
  UpsertMetadataCache error. After a successful gzip store and a failed
  row write, a later TTL hit or stale fallback served the gzip blob with
  the previous row's encoding. On a row-write failure, log it and delete
  the blob just written, so the next request refetches instead.
- fetchOrCacheMetadata read the row once up front and reused it for the
  stale fallback. A request that read an identity row, lost the upstream
  race to a request that stored the gzip blob, and then failed upstream
  labelled the new blob with the old row. Re-read the row before falling
  back so the encoding matches the blob as it is now.

Both only become harmful with an encoding change, which this branch
introduces; the pre-existing validator-from-row read is tracked
separately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ProxyCached loses transfer compression on both hops after #304

2 participants