Repository navigation
Add age-based retention for cached artifacts - #428
Open
pinguinfuss wants to merge 12 commits into
Open
pinguinfuss wants to merge 12 commits into
pinguinfuss wants to merge 12 commits into
Conversation
Cached artifacts stayed forever unless storage.max_size pushed them out.
storage.retention adds a default, per-ecosystem and per-package duration
after which an artifact nobody downloaded gets evicted.
Ecosystems opt in one at a time: a handler registers with
retention.Default, and until it does, configuring that ecosystem (or one
of its packages) fails validation, and the default doesn't touch it. This
commit registers none, so nothing changes yet. Startup logs which
ecosystems the default covers and which it doesn't.
An artifact counts as expired when both its fetch time and its last
access are older than the cutoff. Checking only the last access would
evict a refetched artifact again right away, since clearing a record
keeps its old access time and a cache miss doesn't record a hit.
The sweep scans artifacts in id windows, so no single query holds the
SQLite connection for long, and checks each row against its own rule.
Expired records are cleared and their files go through pending_deletes
instead of being deleted directly, because a buffered hit can make an
artifact that's being served right now look expired. Buffered hits are
flushed before each sweep, and one sweep clears at most 10,000
artifacts.
Since a big first sweep can queue a lot, reclaim now keeps working
through due entries for up to 30 seconds per tick instead of stopping
after 100, and pending_deletes gets an index on queued_at.
Also:
- proxy_artifacts_evicted_total{reason, ecosystem} counts LRU and
retention evictions.
- gradle.build_cache.max_age accepts "7d", as its comment always said.
Refs git-pkgs#306
Describes the new storage.retention block, the order rules apply in, that the default only covers ecosystems that support retention yet, and what to expect from the first sweep over an old cache. Also lists the new eviction counter and notes that gradle max_age takes days. Refs git-pkgs#306
On Postgres, fetched_at and last_accessed_at are TIMESTAMP columns without a zone. They hold the local time the proxy wrote, but lib/pq reads them back labelled UTC. East of UTC that makes every artifact look hours younger than it is, so the sweep evicted late: two hours in Berlin, nine in Tokyo. The candidates' times are now read back as local time, and the database tests run on both SQLite and Postgres, with one that sets the local zone to Asia/Tokyo. A few smaller things: - When a delete fails, reclaim stops for the rest of the minute instead of trying every queued path. Otherwise a storage outage turns into thirty seconds of failed deletes and warnings every minute. - If the buffered hits can't be written, the sweep skips that round. Running on stale access times could clear something that is being downloaded right now. - DefaultCanonical only accepts a package key when rebuilding it gives the same PURL. For alpine and deb it would otherwise produce keys that never match. - sweep_interval has to be at least a minute. - New tests cover the eviction counter and an artifact that gets a hit between the scan and the clear. - The docs no longer say nothing changes without retention, and they point out that the example's ecosystem and package rules only pass validation once those ecosystems support retention. Refs git-pkgs#306
A rule names a whole package. Qualifiers like ?type=pom were dropped on the way to the stored PURL and the rule quietly covered every file of the package. Now such a key fails validation. Refs git-pkgs#306
A package rule for an ecosystem that supports retention, written in a form its packages aren't stored under (pkg:npm/babel/core without the @), failed with "retention is not supported for npm packages". Once the ecosystem is enabled that is no longer true. Such a key now fails with "does not match how npm packages are stored" and points to the package rule forms in the docs. Keys for ecosystems without retention support keep the old message. Refs git-pkgs#306
A mirror run over a package that is still cached goes through the normal cache lookup and records a download, so the package's age starts over. The docs only said that mirrored packages age from the time they were mirrored. Refs git-pkgs#306
Some ecosystems will support retention without package rules, because nothing in their stored records names a package the way a PURL would (conan, helm). A package key for one of them used to fail with "does not match how conan packages are stored; see the package rule forms", which sends people looking for a form that doesn't exist. It now says package rules aren't supported there and points to the ecosystem rule. Refs git-pkgs#306
andrew
requested changes
Oct 11, 2026
andrew
left a comment
andrew
left a comment
Contributor
There was a problem hiding this comment.
Two changes needed before merging:
- In
clearIfExpired, clearing the artifact and queueing deletion are separate writes. If queueing fails or the process exits between them, the file remains on disk without a stored path or deletion entry, so later sweeps cannot retry it. Please commit both operations in one transaction and test rollback on queue failure. proxy_artifacts_evicted_totalis missing from/ui/analyticsandmetricSurface, contrary to CONTRIBUTING.md. Please add its display and initialize it in the coverage test. The default test order misses this, butgo test ./internal/server -run '^Test(EvictionsAreCounted|EveryMetricIsSurfaced)$' -count=2fails because the metric has no home on the analytics page.
Conan gets package rules once its handler stores archives under the recipe name, so the config test that shows the refusal now uses helm, whose cache records name the repository, not the chart. Refs git-pkgs#306
The retention docs said each artifact, each version of a package, ages on its own. A version can have several cached files (a jar and its pom, several wheels, Conan sources and binaries), and each of them ages separately. Refs git-pkgs#306
The sweep cleared the record and queued the file for deletion as two separate writes. If queueing failed, or the process stopped in between, the file stayed in storage with no record pointing at it and no queue entry, and no later sweep would find it again. Both writes now share a transaction, so a failed queue rolls the clear back and the next sweep retries. Refs git-pkgs#306
proxy_artifacts_evicted_total had no place on /ui/analytics. The Runtime card now lists evicted artifacts by reason and ecosystem, without the failure tint, and the coverage test records an eviction so it doesn't depend on test order. Refs git-pkgs#306
Contributor
Author
|
Thanks, both fixed.
While I was in there I noticed main has the same split write in a few places: DiscardArtifact, UpsertArtifact when it replaces a path, and the LRU eviction loop all clear the record and queue or delete the file in separate steps. I left them alone to keep this PR about retention. Happy to send a separate PR that moves them into a transaction too, if you want that. |
fetched_at and last_accessed_at are now written in UTC, and the retention cutoffs are compared in UTC, the same convention git-pkgs#434 uses for the metadata cache. lib/pq sends a time with its offset and a Postgres TIMESTAMP keeps only the wall clock, so UTC on write is what reads back correctly. That replaces reinterpreting the times as local on read. Rows written before this still hold local time. East of UTC they look younger by the offset until they are rewritten, so retention evicts them a few hours late once; west of UTC that much early. Refs git-pkgs#306
Contributor
Author
|
Artifact times (fetched_at, last_accessed_at) are written in UTC, the same convention #434 uses for the metadata cache. Rows written before this change still hold local time: east of UTC they look younger by the offset until they are rewritten, so retention evicts them a few hours late once (west of UTC, that much early). |
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
2 participants
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.
Infrastructure for #306. This adds a
storage.retentionblock (default,ecosystems,packages,sweep_interval) and a sweep that evicts artifacts nobody has downloaded for longer than the configured time.No ecosystem is enabled yet, so this PR changes nothing on its own. Each ecosystem will get its own small PR that registers it from its handler. Until then, naming an ecosystem or package in the config fails validation, and
defaultdoesn't apply to it. On startup the proxy logs which ecosystems retention covers.What's in here:
fetched_atandlast_accessed_atare older than the cutoffpending_deletessweep_intervalmust be at least a minutepending_deletes(queued_at)(migration 010)proxy_artifacts_evicted_total{reason, ecosystem}for LRU and retentiongradle.build_cache.max_ageaccepts"7d"Tested on SQLite and Postgres 16. The new database tests run against both, including one with a non-UTC local zone.