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
Question for @sneak, checked against next at c7173c4: should pixa ever fetch a cached image from its origin again?
Today it never does. CacheTTL (24 hours) is set and never read, so a source and its variants stay cached until the disk cache limit evicts them, and a changed image at the same origin URL is not picked up until then. README.md already tells browsers and proxies the same: an image is served immutable with a max-age of up to one year.
(b) Remove CacheTTL and its constant (recommended): pixa keeps what it fetched until eviction, as README.md already promises; a new image at the origin needs a new URL. Smallest change. #83 then shrinks to passing the origin's Last-Modified on to clients.
(a) Enforce it as a new setting: a cached source older than the limit is checked with the origin again, with a conditional request (the mechanism in #83) so an unchanged image is not downloaded again. The one-year max-age and immutable would then have to come down to the same limit.
Question for @sneak, checked against `next` at `c7173c4`: should pixa ever fetch a cached image from its origin again?
Today it never does. `CacheTTL` (24 hours) is set and never read, so a source and its variants stay cached until the disk cache limit evicts them, and a changed image at the same origin URL is not picked up until then. `README.md` already tells browsers and proxies the same: an image is served `immutable` with a `max-age` of up to one year.
- (b) Remove `CacheTTL` and its constant (recommended): pixa keeps what it fetched until eviction, as `README.md` already promises; a new image at the origin needs a new URL. Smallest change. https://git.eeqj.de/sneak/pixa/issues/83 then shrinks to passing the origin's `Last-Modified` on to clients.
- (a) Enforce it as a new setting: a cached source older than the limit is checked with the origin again, with a conditional request (the mechanism in https://git.eeqj.de/sneak/pixa/issues/83) so an unchanged image is not downloaded again. The one-year `max-age` and `immutable` would then have to come down to the same limit.
https://git.eeqj.de/sneak/pixa/issues/83 waits for this answer; other work goes on.
Model: opus-5-5
sneak
was assigned by clawbot2026-10-04 13:00:05 +02:00
Still current on next at 162a2a3: CacheConfig.CacheTTL is set from DefaultCacheTTL and never read. Decided as the recommended option, by the standing rule to build the recommendation: remove the field and the constant. Enforcing a lifetime would be new caching behaviour, which needs the owner's approval anyway. Disclosure: removing the field takes its line out of the config literals of eight existing tests; no test's checks change.
Model: opus-5-5
Still current on `next` at `162a2a3`: `CacheConfig.CacheTTL` is set from `DefaultCacheTTL` and never read. Decided as the recommended option, by the standing rule to build the recommendation: remove the field and the constant. Enforcing a lifetime would be new caching behaviour, which needs the owner's approval anyway. Disclosure: removing the field takes its line out of the config literals of eight existing tests; no test's checks change.
Model: opus-5-5
Built as decided in #239: CacheConfig.CacheTTL, DefaultCacheTTL and the line that set the field are removed; behaviour is unchanged.
Model: opus-5-5
Built as decided in https://git.eeqj.de/sneak/pixa/pulls/239: `CacheConfig.CacheTTL`, `DefaultCacheTTL` and the line that set the field are removed; behaviour is unchanged.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Verified against
mainat61f42e6.CacheConfig.CacheTTL(internal/imgcache/cache.go:28) is populated in production wiring atinternal/handlers/handlers.go:66withDefaultCacheTTL = 24 * time.Hour(internal/imgcache/module.go:7), and set in six test fixtures. It is never read: grep forconfig.CacheTTL/c.config.CacheTTLacross the tree returns only assignments, no uses.Only
NegativeTTLis actually enforced (cache.go:214), andCleanExpired(cache.go:285-295) purges only thenegative_cachetable.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:
CacheTTLare treated as misses and re-fetched/revalidated. Consider conditional revalidation using upstreamLast-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.Either way:
config.example.ymland README.make checkgreen.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).Question for @sneak, checked against
nextatc7173c4: should pixa ever fetch a cached image from its origin again?Today it never does.
CacheTTL(24 hours) is set and never read, so a source and its variants stay cached until the disk cache limit evicts them, and a changed image at the same origin URL is not picked up until then.README.mdalready tells browsers and proxies the same: an image is servedimmutablewith amax-ageof up to one year.CacheTTLand its constant (recommended): pixa keeps what it fetched until eviction, asREADME.mdalready promises; a new image at the origin needs a new URL. Smallest change. #83 then shrinks to passing the origin'sLast-Modifiedon to clients.max-ageandimmutablewould then have to come down to the same limit.#83 waits for this answer; other work goes on.
Model: opus-5-5
Still current on
nextat162a2a3:CacheConfig.CacheTTLis set fromDefaultCacheTTLand never read. Decided as the recommended option, by the standing rule to build the recommendation: remove the field and the constant. Enforcing a lifetime would be new caching behaviour, which needs the owner's approval anyway. Disclosure: removing the field takes its line out of the config literals of eight existing tests; no test's checks change.Model: opus-5-5
Built as decided in #239:
CacheConfig.CacheTTL,DefaultCacheTTLand the line that set the field are removed; behaviour is unchanged.Model: opus-5-5