Evict remote objects from every cache layer #128

2026-10-04T15:45:41+11:00

unkin-agent commented

2026-10-04 15:45:41 +11:00

Evicting a remote object only deleted its Postgres row. The S3 index and the Redis TTL/ETag keys survived, so stale mirror metadata such as EPEL repodata kept being served.

evict the artifact row, S3 index and Redis keys via Engine.Evict
evict a directory with <dir>/*; other wildcards return 400, unknown remotes 404
wait on the per-path fetch lock for single-path evicts; return 503 on timeout or a Redis error
wildcard evicts take no lock; a fetch already in flight may re-cache its path
return S3 list errors from DeletePrefix instead of deleting nothing

Evicting a remote object only deleted its Postgres row. The S3 index and the Redis TTL/ETag keys survived, so stale mirror metadata such as EPEL repodata kept being served. - evict the artifact row, S3 index and Redis keys via `Engine.Evict` - evict a directory with `<dir>/*`; other wildcards return 400, unknown remotes 404 - wait on the per-path fetch lock for single-path evicts; return 503 on timeout or a Redis error - wildcard evicts take no lock; a fetch already in flight may re-cache its path - return S3 list errors from `DeletePrefix` instead of deleting nothing

unkin-agent added 1 commit 2026-10-04 15:45:42 +11:00

Evict remote objects from every cache layer

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

ci/woodpecker/pr/build Pipeline was successful

Details

5802fd2e2c

DELETE /objects only removed the artifacts row, so mutable indexes (S3
index object + Redis TTL/ETag keys) kept being served. Clear all layers
and treat a trailing * as a prefix.

unkin-agent commented

2026-10-04 15:50:26 +11:00

internal/proxy/engine.go:214 — a bare * gives an empty prefix and wipes every artifact row, index object and TTL/ETag key for the remote, with no guard and no test (previously a no-op) → decide explicitly: reject an empty prefix, or keep it and add a test that pins it.
internal/proxy/evict_test.go:56 — the tests never prove the ETag key is cleared: changingUpstream ignores If-None-Match and always returns 200, so a surviving etag: key would not fail either test → make the upstream honour If-None-Match (304 when it matches) so the stale-ETag path is caught, or assert directly that ForgetPath/ForgetPrefix remove etag: keys.
internal/cache/redis.go:120 — ForgetPath/ForgetPrefix and the globEscaper have no direct test (a remote or path containing *, ?, [) → add a small Redis-backed test.
internal/proxy/engine.go:211 — DB, S3 and Redis are cleared sequentially with Redis last, so a Fetch holding the lock can re-set TTL/ETag from the old upstream response after the delete → nit: clear Redis before S3/DB, or accept and say so.
internal/proxy/engine.go:211 — an unknown remote or nonexistent path still returns 204 since deleting zero rows is not an error → nit: unchanged from before, only fix if the 204 is meant to mean "something was evicted".
internal/server/server.go:204 — /locals/.../objects is now given s.engine although evictLocal never uses it, and NewObjectsHandler(db, nil) panics on a remote DELETE → nit: take the evictor only where evict is routed, or nil-guard.

- internal/proxy/engine.go:214 — a bare `*` gives an empty prefix and wipes every artifact row, index object and TTL/ETag key for the remote, with no guard and no test (previously a no-op) → decide explicitly: reject an empty prefix, or keep it and add a test that pins it. - internal/proxy/evict_test.go:56 — the tests never prove the ETag key is cleared: `changingUpstream` ignores If-None-Match and always returns 200, so a surviving `etag:` key would not fail either test → make the upstream honour If-None-Match (304 when it matches) so the stale-ETag path is caught, or assert directly that `ForgetPath`/`ForgetPrefix` remove `etag:` keys. - internal/cache/redis.go:120 — `ForgetPath`/`ForgetPrefix` and the `globEscaper` have no direct test (a remote or path containing `*`, `?`, `[`) → add a small Redis-backed test. - internal/proxy/engine.go:211 — DB, S3 and Redis are cleared sequentially with Redis last, so a Fetch holding the lock can re-set TTL/ETag from the old upstream response after the delete → nit: clear Redis before S3/DB, or accept and say so. - internal/proxy/engine.go:211 — an unknown remote or nonexistent path still returns 204 since deleting zero rows is not an error → nit: unchanged from before, only fix if the 204 is meant to mean "something was evicted". - internal/server/server.go:204 — `/locals/.../objects` is now given `s.engine` although `evictLocal` never uses it, and `NewObjectsHandler(db, nil)` panics on a remote DELETE → nit: take the evictor only where `evict` is routed, or nil-guard.

unkin-agent added 1 commit 2026-10-04 15:56:34 +11:00

Reject bare * and unknown remotes on evict, serialise with fetch lock

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

ci/woodpecker/pr/build Pipeline was successful

Details

c1c5ff987c

- return 400 for a bare * and 404 for an unknown remote
- take the per-path fetch lock for single-path evicts
- pass the evictor to Routes so local routes no longer carry it
- test If-None-Match revalidation, ForgetPath/ForgetPrefix glob escaping

unkin-agent commented

2026-10-04 15:58:01 +11:00

internal/storage/s3.go:103 — DeletePrefix feeds ListObjects straight into RemoveObjects; minio's removeObjects never inspects ObjectInfo.Err, so a failed listing yields zero deletes and returns nil → 204 with stale index objects left. Range the list channel yourself, return on obj.Err, and feed keys to RemoveObjects.
internal/storage/s3.go:106 — early return res.Err abandons the RemoveObjects result channel, leaking its goroutine → keep draining (record first error, return after the loop).
internal/proxy/engine.go:239 — wildcard branch takes no per-path lock, so an in-flight Fetch can re-set TTL/ETag keys and re-upload the index after the evict, the exact race the single-path branch guards against; this contradicts the PR's stated reason → document it as a known gap in the body/comment, or lock/drain accordingly.
internal/proxy/engine.go:218 — any non-empty prefix is accepted, so 8*, e* or a* deletes every artifact row, index object and Redis key of that remote beginning with that char (rows for immutable RPMs force a full re-download) → require the prefix to end in / (or a minimum depth) and return 400 otherwise.
internal/proxy/engine.go:252 — lock wait: if the lock is not obtained within 30s (or Redis errors) Evict silently proceeds unlocked, and defer ReleaseLock(ctx, ...) uses the request ctx, so a cancelled request leaves the lock held until TTL → release with context.WithoutCancel(ctx); return 409/503 rather than proceeding unlocked on timeout.
internal/proxy/evict_test.go:61 — TestEvictMutableIndexRefetches and the wildcard test still pass if Evict only clears the Redis keys: with the TTL gone, Fetch re-fetches upstream and overwrites the index/artifact row anyway. Nothing fails if DeleteArtifact, store.Delete, DeleteArtifactsByPrefix or DeletePrefix are removed → assert directly that the artifacts row and IndexKey object are absent after Evict (and DeletePrefix spares sibling-prefix and other-remote keys).
internal/proxy/evict_test.go:109 — TestEvictWaitsForFetchLock never checks the lock is released after Evict (dropping the defer ReleaseLock passes) and never covers ctx cancellation or the 30s give-up → assert AcquireLock succeeds after Evict returns; add a cancelled-ctx case.
internal/api/v2/objects_evict_test.go:43 — only the 404 mapping is tested; the 400 mapping and the non-ProxyError → 500 branch are untested → add both cases.

- internal/storage/s3.go:103 — DeletePrefix feeds ListObjects straight into RemoveObjects; minio's removeObjects never inspects `ObjectInfo.Err`, so a failed listing yields zero deletes and returns nil → 204 with stale index objects left. Range the list channel yourself, return on `obj.Err`, and feed keys to RemoveObjects. - internal/storage/s3.go:106 — early `return res.Err` abandons the RemoveObjects result channel, leaking its goroutine → keep draining (record first error, return after the loop). - internal/proxy/engine.go:239 — wildcard branch takes no per-path lock, so an in-flight Fetch can re-set TTL/ETag keys and re-upload the index after the evict, the exact race the single-path branch guards against; this contradicts the PR's stated reason → document it as a known gap in the body/comment, or lock/drain accordingly. - internal/proxy/engine.go:218 — any non-empty prefix is accepted, so `8*`, `e*` or `a*` deletes every artifact row, index object and Redis key of that remote beginning with that char (rows for immutable RPMs force a full re-download) → require the prefix to end in `/` (or a minimum depth) and return 400 otherwise. - internal/proxy/engine.go:252 — lock wait: if the lock is not obtained within 30s (or Redis errors) Evict silently proceeds unlocked, and `defer ReleaseLock(ctx, ...)` uses the request ctx, so a cancelled request leaves the lock held until TTL → release with `context.WithoutCancel(ctx)`; return 409/503 rather than proceeding unlocked on timeout. - internal/proxy/evict_test.go:61 — `TestEvictMutableIndexRefetches` and the wildcard test still pass if Evict only clears the Redis keys: with the TTL gone, Fetch re-fetches upstream and overwrites the index/artifact row anyway. Nothing fails if `DeleteArtifact`, `store.Delete`, `DeleteArtifactsByPrefix` or `DeletePrefix` are removed → assert directly that the `artifacts` row and `IndexKey` object are absent after Evict (and `DeletePrefix` spares sibling-prefix and other-remote keys). - internal/proxy/evict_test.go:109 — `TestEvictWaitsForFetchLock` never checks the lock is released after Evict (dropping the `defer ReleaseLock` passes) and never covers ctx cancellation or the 30s give-up → assert `AcquireLock` succeeds after Evict returns; add a cancelled-ctx case. - internal/api/v2/objects_evict_test.go:43 — only the 404 mapping is tested; the 400 mapping and the non-ProxyError → 500 branch are untested → add both cases.

unkin-agent added 1 commit 2026-10-04 16:06:56 +11:00

Restrict wildcard evict to <dir>/*, return 503 on lock contention, surface S3 list errors

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

e67629cb2d

unkin-agent commented

2026-10-04 16:08:36 +11:00

No findings.

benvin merged commit 6a08539a78 into master

2026-10-04 22:07:15 +11:00

benvin deleted branch benvin/fix-epel-eviction

2026-10-04 22:07:15 +11:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/artifactapi#128