CacheConfig.CacheTTL is configured but never read — cached variants never expire #69

New Issue

clawbot · 2026-08-09T03:45:06+02:00

clawbot commented

2026-08-09 03:45:06 +02:00

Verified against main at 61f42e6.

CacheConfig.CacheTTL (internal/imgcache/cache.go:28) is populated in production wiring at internal/handlers/handlers.go:66 with DefaultCacheTTL = 24 * time.Hour (internal/imgcache/module.go:7), and set in six test fixtures. It is never read: grep for config.CacheTTL / c.config.CacheTTL across the tree returns only assignments, no uses.

Only NegativeTTL is actually enforced (cache.go:214), and CleanExpired (cache.go:285-295) purges only the negative_cache table.

So the "24 hour cache TTL" implied by the constant does not exist: a cached variant is served indefinitely, and a source image that changes at the origin is never re-fetched. For an image proxy that is a correctness question, not just a tidiness one — there is currently no mechanism by which updated upstream content ever reaches a client.

Definition of done

Decide which of these is intended, then make the code say so:

(a) Enforce it — cache entries older than CacheTTL are treated as misses and re-fetched/revalidated. Consider conditional revalidation using upstream Last-Modified/ETag (currently captured nowhere — see the related headers issue) rather than unconditional re-fetch, so a TTL expiry does not mean a guaranteed full re-download.
(b) Remove it — delete the field, the constant, and its fixture uses, and document that pixa caches indefinitely and relies on eviction (#51) plus URL expiry for turnover.

Either way:

No configured-but-ignored field is left behind.
If (a): the TTL becomes a real config key with strict parsing (abort on invalid, per repo policy), documented in config.example.yml and README.
Failing test first: an entry older than the TTL is either re-fetched (a) or explicitly documented-and-asserted as still served (b).
make check green.

Recommendation: (a) — an image proxy that can never pick up an upstream change is surprising, and #51's eviction is size-driven, not freshness-driven, so it does not cover this.

Coordination

Touches internal/imgcache/cache.go, which PR #55 also changes substantially. Do this after #55 merges to avoid a conflict, and check whether #55's accounting timestamps (last_accessed_at) give a cheaper way to implement (a).

Verified against `main` at `61f42e6`. `CacheConfig.CacheTTL` (`internal/imgcache/cache.go:28`) is populated in production wiring at `internal/handlers/handlers.go:66` with `DefaultCacheTTL = 24 * time.Hour` (`internal/imgcache/module.go:7`), and set in six test fixtures. It is **never read**: grep for `config.CacheTTL` / `c.config.CacheTTL` across the tree returns only assignments, no uses. Only `NegativeTTL` is actually enforced (`cache.go:214`), and `CleanExpired` (`cache.go:285-295`) purges only the `negative_cache` table. So the "24 hour cache TTL" implied by the constant does not exist: a cached variant is served indefinitely, and a source image that changes at the origin is never re-fetched. For an image proxy that is a correctness question, not just a tidiness one — there is currently no mechanism by which updated upstream content ever reaches a client. ## Definition of done Decide which of these is intended, then make the code say so: - **(a) Enforce it** — cache entries older than `CacheTTL` are treated as misses and re-fetched/revalidated. Consider conditional revalidation using upstream `Last-Modified`/`ETag` (currently captured nowhere — see the related headers issue) rather than unconditional re-fetch, so a TTL expiry does not mean a guaranteed full re-download. - **(b) Remove it** — delete the field, the constant, and its fixture uses, and document that pixa caches indefinitely and relies on eviction (#51) plus URL expiry for turnover. Either way: 1. No configured-but-ignored field is left behind. 2. If (a): the TTL becomes a real config key with strict parsing (abort on invalid, per repo policy), documented in `config.example.yml` and README. 3. Failing test first: an entry older than the TTL is either re-fetched (a) or explicitly documented-and-asserted as still served (b). 4. `make check` green. **Recommendation: (a)** — an image proxy that can never pick up an upstream change is surprising, and #51's eviction is size-driven, not freshness-driven, so it does not cover this. ## Coordination Touches `internal/imgcache/cache.go`, which PR #55 also changes substantially. Do this **after** #55 merges to avoid a conflict, and check whether #55's accounting timestamps (`last_accessed_at`) give a cheaper way to implement (a).

clawbot added this to the 1.0.0 milestone 2026-08-09 03:45:06 +02:00

clawbot referenced this issue

2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#69