Filter cached packages by ecosystem using pills - #319
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new pill/count data is derived from GetCacheStats() (not aligned with “cached packages” semantics) and the new filter links should URL-encode query values to avoid malformed URLs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This draft/POC updates the “Cached Packages” UI to replace the ecosystem dropdown with clickable “pill” filters and to show only ecosystems that have packages (with per-ecosystem counts), improving navigation and discoverability.
Changes:
- Replaced the ecosystem
<select>with pill-style filter links showing per-ecosystem counts (plus an “All” pill). - Added template helpers for pill styling and a builder to generate the ecosystem filter list.
- Updated the packages list handler and template rendering tests to include the new data fields.
File summaries
| File | Description |
|---|---|
| internal/server/templates/pages/packages_list.html | Replaces ecosystem dropdown with filter pills; adjusts list row layout and sort behavior. |
| internal/server/templates.go | Exposes ecosystemPillClass helper to templates. |
| internal/server/templates_test.go | Updates page render test data; adds tests for ecosystem filter building and pill classes. |
| internal/server/server.go | Populates new TotalPackages / EcosystemFilters fields for the packages list page. |
| internal/server/dashboard.go | Adds pill class helpers and buildEcosystemFilters; extends page data types. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- 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 on lines
+801
to
+808
| var ecosystemFilters []EcosystemFilter | ||
| var totalPackages int64 | ||
| if stats, err := s.db.GetCacheStats(); err != nil { | ||
| s.logger.Error("failed to get cache stats for ecosystem filters", "error", err) | ||
| } else { | ||
| totalPackages = stats.TotalPackages | ||
| ecosystemFilters = buildEcosystemFilters(stats.EcosystemCounts) | ||
| } |
Comment on lines
+21
to
+22
| <a href="/ui/packages?ecosystem={{.Ecosystem}}{{if $.SortBy}}&sort={{$.SortBy}}{{end}}" | ||
| class="{{ecosystemPillClass .Ecosystem}}{{if eq $.Ecosystem .Ecosystem}} ring-2 ring-current ring-offset-1 dark:ring-offset-gray-900{{end}}"> |
Comment on lines
+15
to
+16
| <a href="/ui/packages{{if .SortBy}}?sort={{.SortBy}}{{end}}" | ||
| class="inline-flex items-center gap-1.5 px-3 py-1.5 rounded-full text-xs font-medium bg-gray-100 text-gray-700 dark:bg-gray-800 dark:text-gray-300 hover:opacity-90{{if not .Ecosystem}} ring-2 ring-gray-400 dark:ring-gray-500 ring-offset-1 dark:ring-offset-gray-900{{end}}"> |
Bumps [zizmorcore/zizmor-action](https://github.com/zizmorcore/zizmor-action) from 0.6.2 to 0.6.3. - [Release notes](https://github.com/zizmorcore/zizmor-action/releases) - [Commits](zizmorcore/zizmor-action@3dc1ecc...70fb788) --- updated-dependencies: - dependency-name: zizmorcore/zizmor-action dependency-version: 0.6.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [docker/setup-qemu-action](https://github.com/docker/setup-qemu-action) from 4.2.0 to 4.3.0. - [Release notes](https://github.com/docker/setup-qemu-action/releases) - [Commits](docker/setup-qemu-action@96fe6ef...1f40c72) --- updated-dependencies: - dependency-name: docker/setup-qemu-action dependency-version: 4.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [azure/setup-helm](https://github.com/azure/setup-helm) from 4.3.1 to 5.0.1. - [Release notes](https://github.com/azure/setup-helm/releases) - [Changelog](https://github.com/Azure/setup-helm/blob/main/CHANGELOG.md) - [Commits](Azure/setup-helm@1a275c3...9bc31f4) --- updated-dependencies: - dependency-name: azure/setup-helm dependency-version: 5.0.1 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [modernc.org/sqlite](https://gitlab.com/cznic/sqlite) from 1.57.0 to 1.58.0. - [Changelog](https://gitlab.com/cznic/sqlite/blob/master/CHANGELOG.md) - [Commits](https://gitlab.com/cznic/sqlite/compare/v1.57.0...v1.58.0) --- updated-dependencies: - dependency-name: modernc.org/sqlite dependency-version: 1.58.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…pkgs#336) Bumps [github.com/aws/aws-sdk-go-v2/config](https://github.com/aws/aws-sdk-go-v2) from 1.32.40 to 1.33.2. - [Release notes](https://github.com/aws/aws-sdk-go-v2/releases) - [Commits](aws/aws-sdk-go-v2@config/v1.32.40...config/v1.33.2) --- updated-dependencies: - dependency-name: github.com/aws/aws-sdk-go-v2/config dependency-version: 1.33.2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#330) Bumps [github.com/prometheus/client_model](https://github.com/prometheus/client_model) from 0.6.2 to 0.6.3. - [Release notes](https://github.com/prometheus/client_model/releases) - [Commits](prometheus/client_model@v0.6.2...v0.6.3) --- updated-dependencies: - dependency-name: github.com/prometheus/client_model dependency-version: 0.6.3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [golang.org/x/sync](https://github.com/golang/sync) from 0.22.0 to 0.23.0. - [Commits](golang/sync@v0.22.0...v0.23.0) --- updated-dependencies: - dependency-name: golang.org/x/sync dependency-version: 0.23.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…it-pkgs#334) Bumps [github.com/aws/aws-sdk-go-v2/service/ecr](https://github.com/aws/aws-sdk-go-v2) from 1.61.0 to 1.64.0. - [Release notes](https://github.com/aws/aws-sdk-go-v2/releases) - [Commits](aws/aws-sdk-go-v2@service/s3/v1.61.0...service/s3/v1.64.0) --- updated-dependencies: - dependency-name: github.com/aws/aws-sdk-go-v2/service/ecr dependency-version: 1.63.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ding (git-pkgs#324) * fix(handler): fetch conda repodata gzip-compressed on both hops 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 * fix(handler): pass metadata content-encoding with the body it describes 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. * fix(handler): pin the stale-fallback content-encoding and drop a dead 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. * fix(handler): restore the pre-existing cachedMeta content-encoding field 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. * fix(handler): keep conda's proxyCached and .bz2 route as on main 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. * fix(handler): keep writeMetadataCachedResponse and its callers as on 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. * fix(handler): move the conda gzip change to its own branch 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. * fix(homebrew): fetch the JSON API gzip-compressed on both hops 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. * fix(handler): leave Accept-Encoding unset in proxyMetadataStream for 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. * fix(handler): keep the metadata row and blob from describing different 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. * Drop unused cachedMeta.contentEncoding and fix stale doc reference The field was added by git-pkgs#304 and its only reader is replaced in this branch by the encoding parameter passed alongside the body. The proxyCachedWithEncoding comment named conda repodata, which was moved out of this branch in 5991d95; Homebrew is the caller that ships here. --------- Co-authored-by: Andrew Nesbitt <andrewnez@gmail.com>
* Return the stored artifact from storeArtifact, not a reader storeArtifact returned a CacheResult holding an open file handle. A handle has one read position, so it can only ever serve a single caller, which is what blocks sharing one fetch between concurrent requests. Return the artifact and its storage path instead, and let each caller open its own reader through openStoredArtifact. Threading that type through fetchAndCache, fetchAndCacheFromURL and their error paths is mechanical; behaviour is unchanged. * Coalesce concurrent cache misses A cache miss went from checkCache straight to an upstream fetch with nothing tracking in-flight work, so N concurrent requests for one uncached artifact produced N upstream fetches and N stores to the same key. That is the CI shape: parallel jobs installing overlapping dependencies against a cold cache. The duplicate stores also fail requests, racing fileblob's per-key ".attrs" sidecar into a partial read served as a 502. Over 12 runs of 8 simultaneous requests for one uncached tarball, against bb2205a: before, 8 fetches per run and 12 of 96 responses were 502; after, 1 fetch per run and none failed. Route both miss paths through a shared in-flight map keyed on the artifact, including the download URL and upstream-declared hash so callers expecting different bytes never share a fetch. singleflight does not fit: Do gives waiters no way to leave, while DoChan lets the caller running the fetch abandon it, breaking storeArtifact's scan-on-disconnect contract. Deciding roles under a mutex gives both behaviours. The fetch runs on the first caller's context and is seen through; waiters leave when their own clients do. This removes the sidecar trigger on this path. The race is in fileblob and three writers bypass this path entirely, so it is fixed separately. Fewer failures now reach the circuit breaker, so it trips later. Sixteen concurrent callers against real file:// storage fail 10 of 10 runs on main and pass 10 of 10 here. Other tests pin key discrimination, failure propagation, resolver-path coalescing, per-caller readers, waiter cancellation, key release and panic safety. allocs/op is unchanged. mockStorage gains a mutex so concurrent tests can use it. * Normalize digest case in the coalescing key artifactHashMatches compares digests with strings.EqualFold, but the coalescing key used the hash verbatim. The same digest in two casings produced two keys, so two callers for one artifact each ran their own upstream fetch and store, which is what the coalescing is meant to prevent. * Make the panic coalescing test deterministic The test timed the second caller's arrival with a sleep, so which caller became the leader was left to the scheduler. When it lost that race the second caller ran the fetch itself, and its panic was not recovered, so the test binary died instead of the test failing. Whether a caller has reached the wait is not observable from outside: it runs a cache lookup against the database first, so releasing the leader on a timer races that query. Drive coalesceFetch directly and hold the shared entry instead, which removes the timing entirely. The panicking fetcher is no longer needed. * Recheck the cache before running a shared fetch A caller checks the cache before it reaches coalesceFetch, so a fetch that commits in that gap is invisible to it. Arriving after the sharing entry is gone, it became a new leader and fetched, stored and scanned an artifact the cache already held. The leader now rechecks the committed record first. It serves that record only if its bytes still open, because a record can outlive them, and refetching is the recovery the cache lookup already makes for that case. Waiters are unaffected: the record fills the same shared value a fetch would, and each caller opens its own reader from it. The recheck is the leader's alone. A waiter has a fetch in flight to wait on, and rechecking would race it for no gain. * Lock the mock fetcher's bookkeeping Coalescing tests call the handler from many goroutines. The key keeps the fetch itself serialized, but the mock should not lean on that: it now locks the fields it records, so any concurrency the handler applies is safe under the race detector. * Wait for the leader's fetch instead of sleeping The canceled-waiter test slept 200ms and assumed the leader had taken the key by then. On a slow scheduler the canceled call could become the leader and the test would no longer cover waiter cancellation. The fetcher now signals when its first fetch begins, which happens only once the key is held.
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.83.1 to 1.83.2. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.83.1...v1.83.2) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.83.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Stop writing fileblob's .attrs sidecar fileblob stores blob metadata in an ".attrs" file per object and rewrites it with os.Create, truncating in place outside the atomic rename that protects the blob. A read overlapping a write decodes a partial file and fails with "opening reader: EOF", served as a 502. One writer against four readers on a single key failed 408 of 2000 reads. cacheMetadataBlob is most exposed to it, rewriting a key on every refresh while readers are served from it. Nothing in the proxy reads what the sidecar holds. gocloud.dev/blob is imported only by internal/storage, Store sets no ContentType, and Attributes is used only for Size, which comes from os.Stat. A missing sidecar already defaults cleanly, so "metadata=skip" removes the hazard rather than locking around it, and saves a write per store. * Clear .attrs sidecars left by earlier versions metadata=skip stops fileblob rewriting sidecars but does not delete ones already on disk, so a sidecar left partial by an interrupted write now fails every read of its key for good. Before, a later store repaired it by rewriting. Store therefore removes the sidecar for the key it writes. Removal is atomic where the rewrite was not, so a concurrent reader gets the whole old file or nothing. Delete already removes sidecars, so the two paths drain a cache between them. Deriving that path is necessary because fileblob's key escaping is unexported. It is the identity for a plain key and parts from one only for keys that are not valid local paths, which is what filepath.Localize rejects. That also keeps the removal inside the cache directory: without it a key holding ".." resolves outside. The clearing test runs one key per storage path the proxy builds, seeded through a bucket that still writes sidecars so the path under test is fileblob's own. * Explain why failed sidecar cleanup does not fail the store Move the removal into clearLegacySidecar and say why its error is dropped rather than returned. A failed removal leaves exactly the state this change inherited, while failing the write would turn a cleanup miss into a failed request. Windows makes that concrete: Go opens files with FILE_SHARE_READ and FILE_SHARE_WRITE but not FILE_SHARE_DELETE, so a reader holding the sidecar open blocks deletion, and that reader is the workload this change exists to protect. Propagating would fail stores during exactly the overlap being fixed. The next store of the key retries. A test pins it, using a non-empty directory at the sidecar path to make os.Remove fail with something other than not-exist on any platform. * Fail the concurrency test if its writer stops The writer returned silently when Store failed, so the test could pass with no concurrent writes at all. Its error is now reported, and the test also checks that at least one write completed. Reporting it showed the writer had been dying on Windows at its first collision: Go opens files without FILE_SHARE_DELETE, so a reader holding the file open makes the writer's rename fail with access denied. The test now skips there, since it cannot contend a writer with readers on that platform. * Clear legacy sidecars for keys fileblob escapes legacySidecarPath declined any key filepath.Localize rejects, which on Windows is every key with a colon: OCI digests and Debian epochs. Their sidecars were never cleared there, and a truncated one kept failing reads, since fileblob still reads a sidecar it finds under metadata=skip. fileblob hex-escapes such characters on the way to disk. The path is now derived the same way, so the sidecar is looked for where fileblob wrote it. Localize still validates the escaped form, which keeps the removal inside the cache directory. * Drop stale comment about declining colon keys on Windows 623ff3e made legacySidecarPath escape keys the way fileblob does, so colon-bearing keys are now cleared on Windows and the OCI and Debian rows in TestStoreClearsLegacyAttrsSidecar prove it. The comment described the behaviour before that commit. --------- Co-authored-by: Andrew Nesbitt <andrewnez@gmail.com>
A Debian release is served by more than one archive. Security updates live on
a different host than the main archive, so a single upstream.debian URL cannot
serve a complete suite set and -security suites are unreachable.
Add upstream.debian_repositories, a name-to-URL map served at /debian/{name}/,
modelled on upstream.apk. The field is additive: upstream.debian keeps serving
/debian/pool/... and /debian/dists/... with unchanged cache identities, so
existing deployments and their warm caches are unaffected, and the scalar
field and PROXY_UPSTREAM_DEBIAN are untouched.
Named repositories scope both caches by name, since the same filename can hold
different bytes in different archives. Metadata keys are hashed over name,
upstream URL, and path, as APKHandler does. Names are validated through
validateNamedUpstreams, and "pool" and "dists" are refused because they would
shadow the main archive's own paths.
An unconfigured first path segment stays a main-archive path rather than
returning 404 as the APK handler does: the main archive is unnamed and serves
paths of its own at the root.
* fix(server): tune the shared upstream transport defaults server.serve builds the shared client with safehttp.New, which clones Go's default transport: MaxIdleConnsPerHost stays 0 (an effective limit of two idle connections per host) and ResponseHeaderTimeout stays 0. Handing that client to fetch.NewFetcher via fetch.WithHTTPClient replaces the fetcher's own defaults of 10 idle connections per host and a 60-second response-header timeout. Set both on the shared transport, matching the fetcher defaults: a second burst of concurrent cache misses to one registry now reuses its connections instead of re-dialling most of them, and an upstream that accepts a request but stalls before sending headers is cut off after 60 seconds rather than only by the client's overall timeout. Tests measure connection reuse across two concurrent bursts against a TLS upstream that counts accepted connections (Go default: at most two reused; tuned: all eight) and assert that a stall before headers fails with the response-header timeout. Fixes git-pkgs#327 * fix(server): reconcile http_timeout docs and tighten the transport tests The http_timeout documentation and the config comment said "0" disables the upstream timeout entirely. With a fixed 60-second ResponseHeaderTimeout on the shared transport that is no longer the whole story, so both now say that waiting for response headers stays bounded independently of the setting. Test cleanup from review: the 50ms settle between bursts was dead time (the transport returns a connection to the idle pool before the body's final Read returns, so burst returning already means the pool is settled); the maxNewInBurst sentinel became explicit min/max bounds per case; the stall test dropped the client.Timeout override and the elapsed-time assertion, which was redundant with the error-text check in any realistic run and whose failure message misattributed the cause, and its comment now says plainly that the field assertions pin production while the behavioural half runs at a lowered timeout. * test(server): hold each burst at the upstream instead of sleeping The reuse test kept a burst in flight with a 100ms handler sleep, so a process stall longer than that between spawning the goroutines and their dials let a request finish early and hand its connection to a sibling. Review reproduced this with forced stalls: the default-transport case then dialled 5 instead of 6 connections. The handler now answers only once burstSize requests are waiting at the same time. With HTTP/1.1 pinned that forces every burst onto burstSize distinct connections regardless of scheduling, and the measured counts stay exactly 6 new for Go's default and 0 for the tuned transport, also under the same forced stalls.
* fix(database): set connection pool limits for Postgres OpenPostgres returned sqlx.Open's handle with database/sql's defaults: no cap on open connections and two idle ones. Under load nearly every request opened a new Postgres session, ran its few statements and closed it again, paying a backend fork and SCRAM authentication each time. Set the pool limits the issue suggests: 32 open and 32 idle connections, idle connections closed after 5 minutes and every connection recycled after 30 minutes. The values live in named constants because the mnd linter rejects the literals inline. The test (skipped without PROXY_DATABASE_URL, like the other Postgres tests) takes 16 connections from the pool, releases them and checks that all 16 stay idle; with the default pool only two survive. Fixes git-pkgs#323 * test(database): pin the pool properties instead of the wiring Review pointed out that the pool test compared MaxOpenConnections to the constant it was set from, so the assertion followed the constant and would have accepted postgresMaxOpenConns = 0 (unlimited), and that its burst of 16 only proved MaxIdleConns >= 16. The burst now takes postgresMaxIdleConns connections, so every configured idle slot has to survive the release, and the open cap is checked to be finite and large enough for that burst before any connection is taken, so a cap below the idle count fails fast instead of blocking in db.Conn. The doc comment now says which settings the test covers; the idle-time and lifetime settings only show up in DBStats.MaxIdleTimeClosed and MaxLifetimeClosed after minutes of wall-clock time and stay unexercised. * test(database): guard the pool test against a vacuous burst Review showed that with the burst tied to postgresMaxIdleConns the test also passed for a constant of 2, database/sql's default, or of 0, where it took no connections at all. It now fails outright unless the configured idle count exceeds the default, and every connection it takes is released in a cleanup, so an assertion failure mid-burst no longer leaves sessions open for the rest of the test binary.
…pkgs#349) createTestPostgresDB dropped artifacts, versions, packages and schema_info before calling CreateSchema, but not the migrations table. On a database that has seen one test the migration records survive, so the next CreateSchema fails while recording 001_add_packages_enrichment_ columns with a duplicate key on migrations_pkey. Running the package against one Postgres therefore failed from the second Postgres-backed test on. Drop vulnerabilities, metadata_cache and migrations as well, so the fixture clears every table CreateSchema creates. The package now passes repeatedly against the same database.
The digest-aware cache check discarded a stale entry before its caller took the key. A slow caller could delete an entry another caller's fetch had just committed, and everyone sharing that fetch then failed to open it. The check now only reports the miss. The caller running the shared fetch discards the entry under the key, after the recheck, so one a previous fetch refreshed is served, not deleted. The mismatch warning fires once per refresh instead of once per request. Swift HEAD no longer discards either, having no fetch to do it under. It probes upstream as before, and the next GET replaces the entry. Different digests or URLs, or no digest, use different keys and can still collide on the storage path. That is the storage layout follow-up.
…it-pkgs#347) The 250ms client is meant for the readiness poll, where a timeout is retried. Reusing it for the OCI manifest request makes the test flake under -race on Windows CI when fetch and cache I/O take longer, as seen on git-pkgs#328. The request checks upstream routing, not latency.
* fix(nuget): enforce cooldown across metadata and downloads * fix(nuget): support legacy registration and preserve valid metadata cache * refactor(nuget): address maintainer review cleanup
go install does not apply goreleaser's -ldflags -X, so binaries installed that way reported 'dev'. Read debug.BuildInfo.Main.Version when the ldflag is unset.
The motivation for separate archives was repeated across six files and the reserved-name rationale across five. Keep each in one place -- the reference docs -- and leave the code comments to what an informed reader cannot get from the code: why an unknown first segment is not a 404 as it is for APK, and why the main archive keeps its legacy cache identities. Comments and prose only; no behaviour change.
…it-pkgs#357) Bumps [github.com/aws/aws-sdk-go-v2/service/ecr](https://github.com/aws/aws-sdk-go-v2) from 1.64.0 to 1.65.0. - [Release notes](https://github.com/aws/aws-sdk-go-v2/releases) - [Commits](aws/aws-sdk-go-v2@service/s3/v1.64.0...service/s3/v1.65.0) --- updated-dependencies: - dependency-name: github.com/aws/aws-sdk-go-v2/service/ecr dependency-version: 1.65.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [github.com/git-pkgs/spdx](https://github.com/git-pkgs/spdx) from 0.3.1 to 0.3.2. - [Commits](git-pkgs/spdx@v0.3.1...v0.3.2) --- updated-dependencies: - dependency-name: github.com/git-pkgs/spdx dependency-version: 0.3.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps alpine from 3.24.1 to 3.24.2. --- updated-dependencies: - dependency-name: alpine dependency-version: 3.24.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…it-pkgs#387) * Add storage.passthrough to serve artifacts without storing them With storage.passthrough enabled the proxy streams every artifact from upstream to the client and never writes it to storage or the cache database. Metadata filtering, cooldown and the denylist work as before, so the proxy can sit behind another cache (e.g. an Artifactory remote) purely as a policy layer without holding a second copy of every package. Artifacts whose digest is known up front (OCI, Swift, Helm) are verified while streaming. Their responses are sent chunked and the connection is aborted on a mismatch, so a client never receives a tampered artifact as a complete response. Passthrough is rejected together with scanning, direct_serve and mirror_api, and the mirror command refuses to run with it, since all of them depend on stored artifacts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Address review: abort truncated streams, guard every cache read - serveArtifact aborts the response on any body read error, and when fewer bytes than the declared size were written, not only on a digest mismatch. A failed or short upstream body in passthrough mode no longer reaches the client as a complete 200. - Streamed upstream read failures are logged and counted as stream_failed upstream errors before the response is aborted. - checkCache reports a miss when artifact caching is off, so every cache read path, including Swift archive HEAD requests, ignores entries stored before the mode was enabled. Denylisted versions are still rejected. - Rename storage.passthrough to storage.cache_artifacts (default true, PROXY_STORAGE_CACHE_ARTIFACTS), which names what actually changes. Tests cover an OCI blob shorter than its Content-Length and an npm tarball whose chunked response ends without the final chunk, both served through the HTTP handlers, plus a Swift HEAD request against a cached archive. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Rename Proxy.Passthrough to StreamArtifacts Match the storage.cache_artifacts option: the field says what changes (artifacts are streamed instead of stored) and its zero value keeps the usual caching behaviour for every Proxy built without the server config, such as the mirror command and tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Batch cache hit writes
…rage [1/6] Name every mounted route in requestEcosystem
…-stats # Conflicts: # README.md
…stats [2/6] Per-ecosystem cache and download statistics
[3/6] Add the /ui/analytics page
…kdown [4/6] Report the per-ecosystem breakdown from GET /stats
[5/6] Add a Grafana dashboard
The flag exists to withhold caller addresses. The by-client table carries none, and the same figures are already public at /metrics, so gating it left two metrics with no tile on the page in the default config.
…ribution [6/6] Attribute requests to a caller and a client tool
The npm and Composer handlers rewrite every metadata document they serve so that download URLs point at the proxy: decode the whole document into generic maps, change the URLs, encode it again, and for Composer expand the minified format first. That ran on every request, cached metadata included. With metadata caching on and upstream out of the picture, a cached request still cost 7 ms for @babel/core, 17 ms for symfony/console and 155 ms and 134 MB of allocations for typescript, and an 8 vCPU VM serving cached npm packuments to 20 clients ran the proxy at 450% CPU for about 1,100 requests a second. Rewritten documents are now kept in memory, keyed by the ecosystem, proxy URL, package and a SHA-256 of the raw document, so new bytes from upstream are rewritten again and nothing is served stale. Requests that arrive while a document is being rewritten wait for that rewrite rather than running their own; a waiter leaves when its client does, and the rewrite still completes and is cached. The denylist is fixed at startup, so it needs no place in the key. Cooldown filtering depends on the current time, so with cooldown on the cache is bypassed. metadata_rewrite_cache_size bounds the cache (default "256MB", least recently used out first, "0" to rewrite on every request). NewProxy callers keep the old behaviour unless they set it. Whole cached requests, measured locally: @babel/core 7.1 ms -> 0.38 ms, 54,489 -> 119 allocations typescript 155 ms -> 10.5 ms, 1.37M -> 133 allocations symfony/console 17.4 ms -> 0.48 ms, 165,909 -> 123 allocations What remains is reading the raw document from storage and hashing it.
expandMinifiedVersions deep-copied every inherited field into every version (b68184c) so that rewriting one version's dist URL in place could not change the versions that inherited it. For a package with a long history that means recursively copying require, autoload and the rest hundreds of times per document, on every metadata request. dist is the only field the proxy changes after expansion, so copy just that: rewriteDistURL now gives the version its own dist map before setting the URL, and expansion shares the other inherited values. TestComposerExpandMinifiedSharedDistReferences still guards the original bug, and fails if the copy in rewriteDistURL is removed. On the symfony/console metadata from Packagist the rewritten output is byte-identical, and the rewrite drops from about 20.7 ms to 13.8 ms, with allocations down from 13.6 MB to 9 MB.
…oads (git-pkgs#404) * feat(storage): expose efficient seek for local files Open file-backed artifacts with os.Open so local readers support efficient seeking. Leave cloud storage readers unchanged. Keep the legacy sidecar mapping test focused on fileblob's reader: Blob.Open now reads local files directly and no longer consults the sidecar. Continue checking sidecar cleanup and post-store reads. * feat(handler): preserve efficient seek through integrity checks Keep the cache result's single Reader field and preserve io.Seeker when the underlying storage reader supports efficient seeking. Sequential reads continue through whole-object integrity verification. A successful seek switches subsequent reads to the source for range responses, where full-object verification is not possible. Test both modes and verify the source is closed once. * feat(handler): add request-aware byte range responses Add a request-aware artifact serving helper for bounded, open-ended, and suffix byte ranges, including If-Range handling, 206 responses, and 416 responses for valid unsatisfiable ranges. Ignore malformed and multi-range requests by serving the full artifact. Advertise byte-range support only for seekable readers, preserving HEAD, redirect, and non-seekable behavior. Clarify that net/http recovers ErrAbortHandler per request and keeps the server running. * feat(handler): enable byte ranges across artifact downloads Forward each artifact request to the shared range-aware response helper, including cached generic release assets and OCI blobs. This lets the handler honor ranges only when its reader supports efficient seeking, without changing direct-storage redirect behavior. Add warm-cache endpoint tests that assert partial response headers and bytes without an upstream refetch. Document supported range behavior, unsupported reader fallback, and the integrity limitation of partial reads.
* Coalesce concurrent metadata fetches Concurrent cache misses for one artifact already share a single upstream fetch (git-pkgs#329), but metadata misses did not: every request for a package's metadata went to the registry, even when the same document was already being fetched. In CI that is the common case. Composer resolves versions on every job, since MediaWiki core and its extensions commit no composer.lock, and each package is one /p2/ document, so a burst of jobs fetched each document once per job. Metadata misses now follow the same pattern as artifacts: the first caller fetches, the rest wait for its result, and the caller that takes the key rechecks the cache first so a fetch that just finished is not repeated. The key includes the Accept and Accept-Encoding headers, which change the bytes upstream returns (npm's abbreviated and full documents), and whether the caller validates the response. One difference from artifacts: the shared fetch is detached from the first caller's cancellation. Artifact fetches keep it for scanning and mirroring, neither of which applies to metadata, and otherwise one job disconnecting would fail every job waiting on the same document. runScan detaches the same way, and the HTTP client's timeout still bounds the fetch. This covers every ecosystem that goes through fetchOrCacheMetadata (Composer, npm, PyPI, Maven, Cargo, Swift, Pub, Helm, NuGet), with metadata caching on or off. Streamed metadata with caching off is not coalesced. On an 8 vCPU test instance with caching off, 10 concurrent clients requesting the npm metadata of 1,263 packages went from 12,630 upstream requests to 1,898. With cache_metadata on, 20 clients requesting 1,538 packages made exactly 1,538. * Run each caller's validate on a shared metadata fetch A caller that joined another's metadata fetch got the shared bytes without running its own validate. NuGet's cooldown path uses validate to decode the document for the request, so a joined download found no publication date and was allowed, bypassing the cooldown. coalesceMetadata now reports whether the caller joined, and coalescedMetadataMiss runs that caller's validate on the shared bytes. Validation before caching is unchanged: the first caller still runs it inside the fetch. TestNuGetCooldownConcurrentDownloads reproduces the bypass through NuGetHandler.Routes() with two overlapping downloads of a version inside its cooldown.
Support npm content-addressed tarball URLs
Contributor
|
@kokes do you want to finish this up or mind if I take over? |
* Add cooldown package pattern overrides * Expose full cooldown decisions through pattern policy * Reject unsafe cooldown pattern syntax
Contributor
Author
|
@andrew sorry, got sidetracked by other projects - feel free to take over - I think I've imprinted the general idea into the rough PR, but I'm not married to the implementation. |
andrew
marked this pull request as ready for review
October 5, 2026 07:07
andrew
changed the title
andrew
approved these changes
Oct 5, 2026
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.
I found the dropdown for filtering by ecosystem to be lacking for two reasons:
(There was also the issue of jagged hit counts I addressed for the dashboard in an earlier PR.)
It now looks like this:
Marking this as a draft and a POC, because it's completely vibe coded and I haven't looked at the code to clean it up in any way just yet.