[simonchrz]

Alpine mirrors keep only the current build of each package. After an upgrade, apk add curl=8.5.0-r0 fails even if the proxy still has the old .apk cached, because the current index no longer lists it. This PR lets the proxy keep earlier index versions and serve them by date, so pinned installs keep working.

Opt-in with upstream.apk_snapshots: "90d" (PROXY_UPSTREAM_APK_SNAPSHOTS). Clients then use a dated repository name, e.g. http://proxy/apk/alpine@2026-10-01/v3.22/main in /etc/apk/repositories. /apk/{name}@{YYYY-MM-DD}/… returns the index (or detached signature) that was current at the end of that day (UTC). Days before the first stored version, and days that haven't started yet, return 404. .apk and other files under a snapshot path are handled exactly like the plain path, so they hit the same cached artifacts. A configured repository name that contains @ takes precedence.

How it works: indexes keep going through ProxyCached unchanged. Afterwards, the APK handler reads the metadata cache row. If its digest differs from the newest snapshot, the stored blob is copied to _snapshots/alpine/{key}/{ms}-{digest}, the hash is checked against the row's digest, and a row is added to a new metadata_snapshots table (migration 020) with valid_from_ms. The check and the insert are a single statement, so concurrent requests add one row. The bytes are the signed upstream bytes, so nothing is re-signed. An hourly job (runExclusive) deletes versions that were replaced longer ago than the configured duration. It queues their blobs on pending_deletes, and the existing reclaim loop removes them. The newest version of each index is always kept. GetDuePendingDeletes now also skips paths referenced by metadata_snapshots.

Tested with handler tests using an httptest upstream: two versions served by date, before-first/future/invalid date, unknown repository, a package under a snapshot path shares the cache entry, 304 on If-None-Match, snapshots disabled, and configured name precedence. There are database tests for add/dedup/rollback/lookup and for pruning (including the pending-delete queue), and config tests. A server test wires the config through mountProtocolHandlers with real file storage, to check that the storage hash matches the cached digest.

Open questions:

• Packages listed by kept snapshots are not protected from max_size eviction or alpine retention. The docs say so and suggest package rules with "0". Protecting them would mean parsing each kept APKINDEX/Packages.adb and feeding the referenced files to the retention sweep. That could be a follow-up.
• Snapshot blobs don't count towards storage.max_size. They are bounded by the duration and by the number of indexes clients use.
• The dated path syntax name@date. An alternative is /apk/{name}/snapshot/{date}/…, which would collide with upstream path segments.
• There is no listing of the available snapshot dates (UI/API).
• The SQL is written for Postgres (explicit casts in INSERT … SELECT), but I could only run it against SQLite locally.

[andrew]

Thanks, this looks good overall. One race needs fixing before merge, plus a few smaller things.

internal/handler/apk_snapshots.go: two requests can record the same index version into the same storage path, and the losing one deletes the blob the winner just recorded. The path is {cacheKey}/{ms}-{digest}. When coalesced fetches after an upstream index update finish together, both can call recordSnapshot in the same millisecond with the same digest. Both Store to that one path, one AddMetadataSnapshot inserts, the other gets added=false and discardSnapshotBlob removes the path the inserted row points at. That day's dated path then returns 500 "snapshot unavailable", and since it is the newest snapshot it is never pruned while it stays current. Making the path unique per attempt (e.g. a random suffix in apkSnapshotStoragePath) avoids it. Please add a test where two recorders collide on the path and the recorded blob is still readable afterwards.

The branch conflicts with main in internal/config/config_test.go, where main added TestCooldownShortest next to your new tests. Keeping both sides resolves it.

Smaller items:

• Some comments restate the PR description. The file-level block at the top of internal/handler/apk_snapshots.go can go or shrink to a line. The unattached block near the top of internal/database/metadata_snapshots.go should fold into the MetadataSnapshot doc comment. The comment on the snapshots field in apk.go repeats WithSnapshots.
• internal/server/apk_snapshots.go: if apk_snapshots is turned off after being used, the prune loop exits straight away and the existing metadata_snapshots rows and _snapshots/ blobs stay forever. Either prune everything when it's disabled and rows exist, or note in the docs that disabling doesn't clean up.

[simonchrz]

Fixed in 77525b0. Each recording now stores to a path with a random suffix, so the request whose row isn't added only deletes its own copy. TestAPKSnapshots_SameVersionRecordedTwice records the same version twice in the same millisecond and reads the blob back. It fails without the fix.

• Merged main and kept both tests in config_test.go.
• The comments are trimmed: the file block in apk_snapshots.go is gone (the signed-bytes note moved to WithSnapshots), the database block folded into the MetadataSnapshot doc, and the snapshots field just points to WithSnapshots.
• Disabling: I went with a docs note rather than deleting everything, since a config mistake would otherwise wipe the history. It explains that setting a short duration first prunes all but the newest version.
• After the merge lint flagged serve as too long (maintidx), so the git cache setup moved into configureGitCache.

[andrew]

The random suffix fixes the blob deletion race from my earlier review. There is still a PostgreSQL deduplication race in AddMetadataSnapshot in internal/database/metadata_snapshots.go: putting the check and insert in one INSERT ... SELECT statement does not serialize concurrent callers. Both statements can read the previous digest and insert the same new digest, retaining separate blob copies until pruning.

I reproduced this against local PostgreSQL using the PR's table definition and insert query: one connection inserted without committing, a second connection ran the same insert successfully, and committing both left two rows for the same index and digest. TestAPKSnapshots_SameVersionRecordedTwice calls the recorders sequentially, so it does not cover this case.

Please serialize the latest-digest check and insert per ecosystem/name across replicas, preserving the ability to record a rollback to an earlier digest. Add a concurrent PostgreSQL regression test that verifies one snapshot row remains and its blob is readable.

[simonchrz]

Fixed in ddba210. AddMetadataSnapshot now runs in a transaction that first takes pg_advisory_xact_lock on the ecosystem and name, then does the same check and insert, so a second replica waits until the first commits and then sees its row. Only the newest digest is compared, so a rollback to an earlier digest is still recorded. SQLite keeps the single statement, since it runs one write at a time.

• TestMetadataSnapshots_ConcurrentPostgres reproduces your sequence: one transaction takes the lock and inserts without committing, a second records the same digest. It checks that the second waits, isn't added after the commit, and that one row remains. It fails on the previous query. It also checks that going back to an earlier digest is still recorded.
• TestAPKSnapshots_ConcurrentReplicas records the same index from two proxies sharing the Postgres database, 16 times at once, and checks for one row, a readable blob, and no leftover copies.

Main is merged in.

[andrew]

internal/handler/apk.go:172-174 records a snapshot only after ProxyCached finishes writing the response, and recordSnapshot in internal/handler/apk_snapshots.go:109 then rereads the mutable metadata cache row. A concurrent refresh can replace that row and blob before the first request records its version.

I reproduced this through the APK handler with metadata caching and snapshots enabled and metadata_ttl set to 0: pause the first response while it writes index v1, change upstream to v2, let a second request finish, then resume the first response. Both requests return 200 with their respective index bytes, but only one snapshot row remains, for v2. The first request rereads v2 and skips recording because that digest already exists. The v1 history is permanently lost.

Please preserve the exact document and its observation time before a concurrent refresh can replace it, rather than reconstructing the snapshot from the cache after writing the response. Add an overlapping-request regression test that verifies both served versions are retained with readable snapshot blobs.

[simonchrz]

Fixed in 04f732e. The snapshot is now recorded from the bytes the request serves, before the response is written, with the time they were fetched, instead of rereading the cache row afterwards. A cache hit records the row it read before serving, after checking its digest. AddMetadataSnapshot now compares a new snapshot with the version current at its own time rather than the newest one, so a version recorded after a newer one keeps its place in the history.

TestAPKSnapshots_OverlappingRequests follows your sequence through the handler: the first request pauses while writing v1, upstream changes to v2, a second request serves v2, then the first finishes. It checks that both versions are kept in order with readable blobs. TestMetadataSnapshots_RecordedLate covers the database side on SQLite and Postgres. Both fail on the previous code.

[andrew]

The overlapping-request fix works, and TestAPKSnapshots_OverlappingRequests covers it. One gap from this round, plus a conflict with main.

internal/handler/apk_snapshots.go:121-127: the cache-hit branch (freshMetadataEntry, then serveCachedMetadata, then recordStoredSnapshot) has no test. Every handler test builds its handler through newSnapshotAPKHandler, which sets MetadataTTL = 0, so this branch never runs. With the default metadata_ttl of 5m it is the path most index requests take in production. Please add a handler test with a TTL above 0: store a cached index row before snapshots are enabled, make one request through the plain path, and check that a snapshot row is added and its blob reads back. Also check that a hit on a version already recorded adds no row.

The branch conflicts with main in internal/server/server.go. Main now calls startRetentionLoop(bgCtx, retentionResolvers). Keep that line and add the startAPKSnapshotPruning goroutine after it.

Smaller items:

• serveMetadataWithSnapshot (apk_snapshots.go:113-136) copies the body of proxyCachedWithEncoding (handler.go:2156-2174) with recording added, so later changes to ProxyCached would miss APK indexes when snapshots are on. Please give proxyCachedWithEncoding optional hooks for a cache hit and a fetched body, and call it from the APK handler instead of keeping a copy.
• The comments added this round are longer than they need to be. The AddMetadataSnapshot doc (metadata_snapshots.go:63-74) can come down to the advisory-lock reason and the casts note, since the "current at that time" and rollback sentences say the same thing. The serveMetadataWithSnapshot doc (apk_snapshots.go:108-112) describes the history of the fix, and one line is enough. The inline comments at apk_snapshots.go:118-120 and 132-133 repeat the function docs and can go.

[simonchrz]

Fixed in 143eef7, with main merged in 7b91897.

• TestAPKSnapshots_CacheHit runs with a 5m TTL: it caches the index with snapshots off, then checks that a hit adds a snapshot whose blob reads back as the cached version, and that a second hit adds no row. It fails if the hit path doesn't record.
• proxyCachedWithEncoding takes optional metadataHooks for a cache hit and a fetched body. The APK handler calls it with both, so its copy is gone. Other callers pass nil.
• Shortened the AddMetadataSnapshot and serveMetadataWithSnapshot docs and dropped the inline comments.
• server.go: kept startRetentionLoop(bgCtx, retentionResolvers) and added startAPKSnapshotPruning after it.