feat(proxies): cache GitHub release artifacts and Packagist - #202
Merged
Merged
Conversation
Adds two CacheProxy implementations and makes ServeArtifact honour TTL. WHY THESE TWO An inventory of the fleet's workflows found the largest uncached ecosystems were not the ones with proxies. Files per ecosystem across the org: cargo build 39 (proxy existed, was switched off -- now enabled) composer install 22 (no proxy on main) releases/download 19 (no proxy anywhere) dtolnay/rust-toolchain 11 (served by the cargo proxy) go mod download 4 (the one ecosystem that HAD a cache) Measured cost on a real build (php-sdk, x86_64 gnu, ducks, 2026-09-20): 40 source artifacts took 87s of a 591s build. GHREL GitHub releases are a distribution channel with no registry, and no redirect of the GOPROXY kind: a release download is a plain github.com URL baked into whatever tool fetches it. Intercepting it transparently would mean terminating TLS for job containers -- a CA in every image and a real change to what untrusted job code is exposed to. So this advertises GHREL_PROXY instead and lets a tool opt in; unset, everything goes upstream exactly as before. Metadata is cached with a TTL; asset URLs inside it are rewritten to the proxy's /download/ route, because otherwise a caller takes kilobytes of JSON from the LAN and every megabyte from the CDN. COMPOSER Rewritten on pkgcache rather than lifting the implementation from the stale feat/cargo-composer-cache branch: that version predates pkgcache entirely and would have reinvented the bounded LRU store, traversal-safe key mapping and fail-open fetch -- and disk-full is a failure mode these nodes have hit twice. It also fixes what that version documented about itself: "distribution zips come from mirrors and GitHub, which this proxy does not front". Metadata is the cheap part. Packagist dist URLs name an exact commit, so they are genuinely immutable and are now fronted and cached permanently. The archive route re-checks the host allowlist so a job cannot hand it an arbitrary URL and use ephemerd as an open proxy. SERVEARTIFACT NOW HONOURS TTL ServeArtifact served any cached entry unconditionally -- it never consulted req.TTL. That is correct for every existing caller: npm, PyPI and pub all refuse to re-publish a file, so their proxies pass Immutable and the question never arises. GitHub does not: an asset can be deleted and re-uploaded under the same URL. Without this, ghrel's non-immutable default was a no-op and the safe default was silently "cache forever" -- the exact hazard it exists to avoid. A stale, non-immutable entry is now revalidated with a conditional GET, reusing decide(), the same primitive Document already uses. Gated so Immutable callers are untouched; npm/pip/pub/cargo suites unchanged. Tests cover URL rewriting for both proxies (single-object AND array release shapes), the composer open-proxy guard, cache-key safety, and both TTL branches. Revert-verified: neutering revalidation gives "conditional requests = 0, want 2"; neutering either rewrite fails its suite; removing the composer allowlist check lets a metadata-service URL through with 200 instead of 403.
|
ePHPm Preview — removed Preview deployment has been torn down. |
CI failed with 23 bodyclose violations that go vet and go test do not catch --
golangci-lint runs that linter, mage lint does not.
The first attempt was to have get() close the body itself via t.Cleanup. That
does not work: bodyclose flags the CALL SITE of anything returning a response
it cannot see closed, so a helper that hands one back can never satisfy it
however careful the caller is.
So get() no longer returns *http.Response. It reads and closes the body and
returns a small result{code, data}, which is all these tests ever used. No
response escapes the helper, and body() became redundant and is deleted.
Verified with the linter CI actually runs (mage lint), not just go vet:
bodyclose 21 -> 0. The single remaining staticcheck hit is in
pkg/networking/network_windows_test.go, which this branch does not touch and
which Linux CI never builds -- it is why the original failure listed only
bodyclose.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two new
CacheProxyimplementations, plus a fix that makesServeArtifacthonourTTL.Why these two
An inventory of the org’s workflows found the largest uncached ecosystems were not the ones with proxies:
cargo buildcomposer installreleases/downloaddtolnay/rust-toolchaingo mod downloadMeasured cost on a real build (php-sdk, x86_64 gnu, 2026-09-20): 40 source artifacts, 87s of a 591s build.
ghrel
GitHub releases are a distribution channel with no registry and no
GOPROXY-style redirect — a release download is a plaingithub.comURL baked into whatever tool fetches it. Intercepting transparently would mean terminating TLS for job containers (a CA in every image, and a real change to what untrusted job code is exposed to).So it advertises
GHREL_PROXYand lets tools opt in. Unset, everything goes upstream exactly as today — which also keeps builds correct on GitHub-hosted runners and laptops.Metadata is cached with a TTL and its asset URLs rewritten to the proxy’s
/download/route — otherwise a caller takes kilobytes of JSON from the LAN and every megabyte from the CDN.composer
Deliberately rewritten on
pkgcacherather than lifting the implementation from the stalefeat/cargo-composer-cachebranch. That version predatespkgcacheentirely and would have reinvented the bounded LRU store, traversal-safe key mapping and fail-open fetch — and disk-full is a failure mode these nodes have hit twice.It also fixes what that version documented about itself:
Metadata is the cheap part. Packagist dist URLs name an exact commit, so they are genuinely immutable — now fronted and cached permanently. The archive route re-checks the host allowlist, so a job cannot hand it an arbitrary URL and use ephemerd as an open proxy.
ServeArtifactnow honoursTTLThis is the part most worth reviewing.
ServeArtifactserved any cached entry unconditionally — it never consultedreq.TTL. That is correct for every existing caller: npm, PyPI and pub all refuse to re-publish a file, so their proxies passImmutableand the question never arises.GitHub does not. An asset can be deleted and re-uploaded under the same URL. Without this, ghrel’s non-immutable default was a no-op and the safe default was silently "cache forever" — the exact hazard it exists to avoid.
A stale, non-immutable entry is now revalidated with a conditional GET, reusing
decide()— the same primitiveDocumentalready uses. Gated soImmutablecallers are untouched; npm/pip/pub/cargo suites are unchanged.Testing
go build,go vet, andgo test ./pkg/proxies/... ./pkg/config/... ./cmd/...all pass.Coverage: URL rewriting for both proxies (single-object and array release shapes), the composer open-proxy guard, cache-key safety, and both TTL branches.
Revert-verified — each of these was confirmed to fail when the behaviour is neutered:
conditional requests = 0, want 2Notes for reviewers
?(rejected bySafeSegments, so paginated listings went upstream forever), nested API paths colliding file-vs-directory, a composer dist key embedding a host with a port, andadvertiseBase()breaking on a port-0 listener. All failed silently —Document()logs a warning and serves the bytes — so the cache would have appeared to work while caching nothing.