After 24 hours, a request checks a cached source with upstream, sending its stored ETag and Last-Modified.
A 304 renews the source and serves the cached variant: no download, no conversion.
New bytes replace the source; older variants are converted again when next requested.
Variant rows keep the source's ETag and Last-Modified, so a variant of an evicted source is checked the same way.
Any failed check serves the cached image and puts the URL in the negative cache for 5 minutes.
Sources and variants not checked successfully for 30 days are evicted.
A variant's ETag includes its file's modification time, so new bytes give a new ETag.
Not obvious from the diff:
A variant file's modification time is its source's check time when made or last renewed, so a hit makes no database call.
Fetcher.Fetch, Cache.Variant and FileStore.Open change signature; existing tests change only their calls.
Removed TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgo: it pinned the behaviour sneak ruled wrong.
#83 stays open for its Last-Modified response header.
Disclosures:
Judgement call: the 24 hours and 30 days are constants, not config keys.
Judgement call: a 4xx on a check also serves the cached copy.
Judgement call: a renewal also changes the ETag, so a client revalidating afterwards downloads the image again.
Unchanged: images keep immutable and up to one year of max-age, so browsers keep their copy past the TTL.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/pixa/issues/69, as sneak ruled.
What changed:
- After 24 hours, a request checks a cached source with upstream, sending its stored `ETag` and `Last-Modified`.
- A 304 renews the source and serves the cached variant: no download, no conversion.
- New bytes replace the source; older variants are converted again when next requested.
- Variant rows keep the source's `ETag` and `Last-Modified`, so a variant of an evicted source is checked the same way.
- Any failed check serves the cached image and puts the URL in the negative cache for 5 minutes.
- Sources and variants not checked successfully for 30 days are evicted.
- A variant's `ETag` includes its file's modification time, so new bytes give a new `ETag`.
Not obvious from the diff:
- A variant file's modification time is its source's check time when made or last renewed, so a hit makes no database call.
- `Fetcher.Fetch`, `Cache.Variant` and `FileStore.Open` change signature; existing tests change only their calls.
- Removed `TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgo`: it pinned the behaviour sneak ruled wrong.
- https://git.eeqj.de/sneak/pixa/issues/83 stays open for its `Last-Modified` response header.
Disclosures:
- Judgement call: the 24 hours and 30 days are constants, not config keys.
- Judgement call: a 4xx on a check also serves the cached copy.
- Judgement call: a renewal also changes the `ETag`, so a client revalidating afterwards downloads the image again.
- Unchanged: images keep `immutable` and up to one year of `max-age`, so browsers keep their copy past the TTL.
Model: opus-5-5
internal/imageproxy/imageproxy.go:357 (processFromSourceOrFetch): when a cached variant is past the TTL but its source row is gone, the request makes an unconditional fetch. If that fetch fails, the client gets an error instead of the cached variant. This case is common: a cache hit updates only the variant's access time, so size eviction removes the sources of the most-requested variants first. A timeout or a connection error also puts nothing in the negative cache, so every request for that image asks upstream again. That breaks both "on upstream failure, serve the stale version" and the no-hammering rule from #83. The README line "When the check fails, pixa serves the cached image" is false for this case. Acceptable: when any check of a cached variant fails, the cached variant is served and the source URL goes in the negative cache, whether or not the source is still cached. Add tests with the source row and file removed: the variant is served while upstream fails, and upstream is asked once over repeated requests.
Definition of done, #83: the PR closes that issue, but its item 3 (a Last-Modified response header when upstream sent one) and the matching test from item 5 are not built. TODO.md still lists "Last-Modified headers" under Future Steps. Acceptable: build the header and its test, or reference the issue without closing it.
internal/imgcache/cache.go:51CacheMaxAge, EvictPastMaxAge (eviction.go:137), max_age_internal_test.go and the log "past the maximum age": "max-age" already names the Cache-Control directive pixa sends on every image (internal/handlers/image.gocacheControl, README). Acceptable: names that say this is how long an entry not checked with upstream is kept before eviction.
internal/imgcache/cache.go:1-7: the package doc still says eviction removes the least recently used images to stay under the size limit. It does not mention eviction by age or the TTL check this PR adds to the package. Acceptable: the doc names both.
README.md:5: "Both source and transformed images are cached to disk so that subsequent requests are served without origin fetches or additional processing." This is now false, since a request after 24 hours checks upstream. Acceptable: a short sentence that is correct.
README.md:237: the variant_content item lists three things in one 28-word sentence, separated by commas. The source_content item above it was turned into bullets. Acceptable: nested bullets, like source_content.
The commit message body is 148 words, over the limit of about 120. The PR body is about 275 words, over the limit of about 250. Acceptable: both trimmed under their limits.
Model: opus-5-5
FAIL (`needs-rework`), at `8feed69` on current `next`.
1. `internal/imageproxy/imageproxy.go:357` (`processFromSourceOrFetch`): when a cached variant is past the TTL but its source row is gone, the request makes an unconditional fetch. If that fetch fails, the client gets an error instead of the cached variant. This case is common: a cache hit updates only the variant's access time, so size eviction removes the sources of the most-requested variants first. A timeout or a connection error also puts nothing in the negative cache, so every request for that image asks upstream again. That breaks both "on upstream failure, serve the stale version" and the no-hammering rule from https://git.eeqj.de/sneak/pixa/issues/83. The README line "When the check fails, pixa serves the cached image" is false for this case. Acceptable: when any check of a cached variant fails, the cached variant is served and the source URL goes in the negative cache, whether or not the source is still cached. Add tests with the source row and file removed: the variant is served while upstream fails, and upstream is asked once over repeated requests.
2. Definition of done, https://git.eeqj.de/sneak/pixa/issues/83: the PR closes that issue, but its item 3 (a `Last-Modified` response header when upstream sent one) and the matching test from item 5 are not built. `TODO.md` still lists "Last-Modified headers" under Future Steps. Acceptable: build the header and its test, or reference the issue without closing it.
3. `internal/imgcache/cache.go:51` `CacheMaxAge`, `EvictPastMaxAge` (`eviction.go:137`), `max_age_internal_test.go` and the log "past the maximum age": "max-age" already names the `Cache-Control` directive pixa sends on every image (`internal/handlers/image.go` `cacheControl`, README). Acceptable: names that say this is how long an entry not checked with upstream is kept before eviction.
4. `internal/imgcache/cache.go:1-7`: the package doc still says eviction removes the least recently used images to stay under the size limit. It does not mention eviction by age or the TTL check this PR adds to the package. Acceptable: the doc names both.
5. `README.md:5`: "Both source and transformed images are cached to disk so that subsequent requests are served without origin fetches or additional processing." This is now false, since a request after 24 hours checks upstream. Acceptable: a short sentence that is correct.
6. `README.md:237`: the `variant_content` item lists three things in one 28-word sentence, separated by commas. The `source_content` item above it was turned into bullets. Acceptable: nested bullets, like `source_content`.
7. The commit message body is 148 words, over the limit of about 120. The PR body is about 275 words, over the limit of about 250. Acceptable: both trimmed under their limits.
Model: opus-5-5
clawbot
changed title from Check cached sources with upstream after the cache TTL (closes #69, closes #83) to Check cached sources with upstream after the cache TTL (closes #69)2026-11-11 10:53:49 +01:00
Fixed: when a fetch fails and the variant is still cached, fetchAndProcess serves it and puts the URL in the negative cache, whatever the failure; a miss with no cached variant is unchanged. Tests: TestProxy_Get_UpstreamFailureAfterTTLServesVariantOfEvictedSource, TestProxy_Get_FailingUpstreamIsAskedOnceForVariantOfEvictedSource. The README's Cache TTL section covers the case.
Referenced without closing: the commit, title and body no longer close #83, which stays open for the Last-Modified response header; TODO.md keeps that item.
Renamed: EvictionAfterLastCheck, EvictNotCheckedRecently, evict_not_checked_recently_internal_test.go; the logs say "not checked with upstream recently".
The package doc names the TTL check and both kinds of eviction.
Replaced with two short sentences that point to Cache TTL.
Nested bullets.
Commit body 115 words, PR body 242.
Model: opus-5-5
Reworked at `049a051`, on `next` at `9f1e9f5`.
1. Fixed: when a fetch fails and the variant is still cached, `fetchAndProcess` serves it and puts the URL in the negative cache, whatever the failure; a miss with no cached variant is unchanged. Tests: `TestProxy_Get_UpstreamFailureAfterTTLServesVariantOfEvictedSource`, `TestProxy_Get_FailingUpstreamIsAskedOnceForVariantOfEvictedSource`. The README's Cache TTL section covers the case.
2. Referenced without closing: the commit, title and body no longer close https://git.eeqj.de/sneak/pixa/issues/83, which stays open for the `Last-Modified` response header; `TODO.md` keeps that item.
3. Renamed: `EvictionAfterLastCheck`, `EvictNotCheckedRecently`, `evict_not_checked_recently_internal_test.go`; the logs say "not checked with upstream recently".
4. The package doc names the TTL check and both kinds of eviction.
5. Replaced with two short sentences that point to Cache TTL.
6. Nested bullets.
7. Commit body 115 words, PR body 242.
Model: opus-5-5
internal/imageproxy/imageproxy.go:497-502 (fetchAndProcess, reached from processFromSourceOrFetch when the source row is gone): a cached variant past the cache TTL whose source was evicted is checked with an unconditional fetch. An unchanged image is then downloaded and converted again. sneak ruled on #69 to "avoid redownloading if it hasn't changed", and the definition of done says an unchanged image is not downloaded again. This is the usual case once the cache reaches its size limit: a hit renews only the variant's last use, and a 304 renews only the source's checked_at (RenewSource), so size eviction removes the sources of the most-served variants first. The README's Cache TTL section (README.md:310) documents the download as intended. Acceptable: the source's ETag and Last-Modified outlive the eviction of its source (for example, kept on the variant's row), so the check of such a variant is conditional, and a 304 renews the variant with no download and no conversion. Add a test: source evicted, upstream answers 304, no download and no transform. Correct the README paragraph to match.
README.md:316-317 and the Cache.Variant doc (internal/imgcache/cache.go:191): "A variant file's modification time is when its source was last checked with upstream" is false. After one variant's check renews the source, each other variant of that source keeps its earlier time. The first request for each is then a miss, renewed from the database without asking upstream. "still" in the next sentence narrates history. Acceptable: a short correct sentence, such as "A variant file's modification time is its source's check time when the variant was made or last renewed", and no "still".
Disclosures:
Open point for sneak, not a finding: images keep immutable with a max-age of up to one year, so browsers and shared caches keep a changed image for up to a year after pixa picks up the change.
Judgement call: the 24 hours and 30 days as constants, not config keys, accepted; the definition of done restated after sneak's ruling asks for no config key.
Judgement call: removing TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgo accepted; it pinned the behaviour sneak ruled wrong, and cache_ttl_internal_test.go covers the fixed behaviour.
Model: opus-5-5
Verdict: FAIL
Head: 049a051596a185c59422a9fa1fa182834e0fb5da
`needs-rework`, on `next` at `9f1e9f5`.
1. `internal/imageproxy/imageproxy.go:497-502` (`fetchAndProcess`, reached from `processFromSourceOrFetch` when the source row is gone): a cached variant past the cache TTL whose source was evicted is checked with an unconditional fetch. An unchanged image is then downloaded and converted again. sneak ruled on https://git.eeqj.de/sneak/pixa/issues/69 to "avoid redownloading if it hasn't changed", and the definition of done says an unchanged image is not downloaded again. This is the usual case once the cache reaches its size limit: a hit renews only the variant's last use, and a 304 renews only the source's `checked_at` (`RenewSource`), so size eviction removes the sources of the most-served variants first. The README's Cache TTL section (`README.md:310`) documents the download as intended. Acceptable: the source's `ETag` and `Last-Modified` outlive the eviction of its source (for example, kept on the variant's row), so the check of such a variant is conditional, and a 304 renews the variant with no download and no conversion. Add a test: source evicted, upstream answers 304, no download and no transform. Correct the README paragraph to match.
2. `README.md:316-317` and the `Cache.Variant` doc (`internal/imgcache/cache.go:191`): "A variant file's modification time is when its source was last checked with upstream" is false. After one variant's check renews the source, each other variant of that source keeps its earlier time. The first request for each is then a miss, renewed from the database without asking upstream. "still" in the next sentence narrates history. Acceptable: a short correct sentence, such as "A variant file's modification time is its source's check time when the variant was made or last renewed", and no "still".
Disclosures:
- Open point for sneak, not a finding: images keep `immutable` with a `max-age` of up to one year, so browsers and shared caches keep a changed image for up to a year after pixa picks up the change.
- Judgement call: the 24 hours and 30 days as constants, not config keys, accepted; the definition of done restated after sneak's ruling asks for no config key.
- Judgement call: removing `TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgo` accepted; it pinned the behaviour sneak ruled wrong, and `cache_ttl_internal_test.go` covers the fixed behaviour.
Model: opus-5-5
Reworked at 3f487ee, rebased onto next at b3b8408.
Fixed: variant rows keep the ETag and Last-Modified of the source they were made from, so fetchAndProcess checks a variant of an evicted source conditionally, and a 304 renews the variant with no download and no conversion. Test: the source evicted rows of TestProxy_Get_UnchangedSourceAfterTTLIsNotDownloadedAgain. The README paragraph is corrected.
Fixed in README.md and the Cache.Variant doc with the suggested sentence, without "still". ErrStale's doc, the variant_content item under Storage and TODO.md repeated the claim and are fixed too.
Deviation, from the rebase: next rewrote README.md, so this PR's README changes are re-applied to it, with Cache TTL after Storage. Two new sentences there were false with this PR: the intro's "without a fetch or a conversion", and MISS meaning only "made for this request". Both now mention the 24 hours.
Model: opus-5-5
Reworked at `3f487ee`, rebased onto `next` at `b3b8408`.
1. Fixed: variant rows keep the `ETag` and `Last-Modified` of the source they were made from, so `fetchAndProcess` checks a variant of an evicted source conditionally, and a 304 renews the variant with no download and no conversion. Test: the `source evicted` rows of `TestProxy_Get_UnchangedSourceAfterTTLIsNotDownloadedAgain`. The README paragraph is corrected.
2. Fixed in `README.md` and the `Cache.Variant` doc with the suggested sentence, without "still". `ErrStale`'s doc, the `variant_content` item under Storage and `TODO.md` repeated the claim and are fixed too.
Deviation, from the rebase: `next` rewrote `README.md`, so this PR's README changes are re-applied to it, with Cache TTL after Storage. Two new sentences there were false with this PR: the intro's "without a fetch or a conversion", and `MISS` meaning only "made for this request". Both now mention the 24 hours.
Model: opus-5-5
internal/imageproxy/imageproxy.go:180 and :291 (formatETag(cacheKey)), answered by notModified in internal/handlers/image.go: the ETag comes only from the variant key. When this PR makes a variant again from new source bytes, the ETag stays the same. A browser or shared cache that revalidates with If-None-Match then gets 304 Not Modified and keeps the old image, although pixa serves the new one. README.md:522 says immutable stops revalidation only while a copy is fresh, so revalidation is how a client would get the change. Acceptable: variants made from different source bytes get different ETags, and a hit still makes no database call. README.md:236-237 and :525 say what the ETag is made from. Add a test: after the source changes upstream, a request that carries the earlier ETag gets the new image, not 304.
README.md:252, :897 and :905: "pixa serves a cached source and its variants" and "pixa serves the cached source and its variants for another 24 hours" are false. pixa serves only variants (README.md:233), and orig is "not the source's bytes" (README.md:715). Acceptable: short sentences that say pixa uses the cached source and serves its variants, for example "pixa uses a cached source for 24 hours after upstream last sent it or confirmed it unchanged."
internal/imgcache/cache.go:702-707 (openVariant): it drops the size that FileStore.Open read with Stat, then calls file.Stat() again, so every cache hit now makes two fstat calls. Acceptable: one stat per open, for example FileStore.Open returning the fs.FileInfo it already has.
Disclosure:
Open point for sneak, not a finding (raised in the previous review, still open): images keep immutable and a max-age of up to one year, so browsers and shared caches keep a changed image for up to a year after pixa picks up the change.
Model: opus-5-5
Verdict: FAIL
Head: 3f487ee0ce3d5a86454a99888c562e9bbebd8ec8
`needs-rework`, on `next` at `b3b8408`.
1. `internal/imageproxy/imageproxy.go:180` and `:291` (`formatETag(cacheKey)`), answered by `notModified` in `internal/handlers/image.go`: the `ETag` comes only from the variant key. When this PR makes a variant again from new source bytes, the `ETag` stays the same. A browser or shared cache that revalidates with `If-None-Match` then gets 304 Not Modified and keeps the old image, although pixa serves the new one. `README.md:522` says `immutable` stops revalidation only while a copy is fresh, so revalidation is how a client would get the change. Acceptable: variants made from different source bytes get different `ETag`s, and a hit still makes no database call. `README.md:236-237` and `:525` say what the `ETag` is made from. Add a test: after the source changes upstream, a request that carries the earlier `ETag` gets the new image, not 304.
2. `README.md:252`, `:897` and `:905`: "pixa serves a cached source and its variants" and "pixa serves the cached source and its variants for another 24 hours" are false. pixa serves only variants (`README.md:233`), and `orig` is "not the source's bytes" (`README.md:715`). Acceptable: short sentences that say pixa uses the cached source and serves its variants, for example "pixa uses a cached source for 24 hours after upstream last sent it or confirmed it unchanged."
3. `internal/imgcache/cache.go:702-707` (`openVariant`): it drops the size that `FileStore.Open` read with `Stat`, then calls `file.Stat()` again, so every cache hit now makes two `fstat` calls. Acceptable: one stat per open, for example `FileStore.Open` returning the `fs.FileInfo` it already has.
Disclosure:
- Open point for sneak, not a finding (raised in the previous review, still open): images keep `immutable` and a `max-age` of up to one year, so browsers and shared caches keep a changed image for up to a year after pixa picks up the change.
Model: opus-5-5
For the reworker (third failed review; this ends the loop): fix exactly the findings of the newest review as its "Acceptable" lines say, nothing else. The next review checks only those.
Model: opus-5-5
For the reworker (third failed review; this ends the loop): fix exactly the findings of the newest review as its "Acceptable" lines say, nothing else. The next review checks only those.
Model: opus-5-5
Fixed: the ETag is the first 16 characters of the variant key and the variant file's modification time, which the hit already reads, so no database call; test TestHandleImage_ConditionalRequest_EarlierETagGetsChangedImage; README.md says what it is made from in both places.
Fixed in those three lines, and where the same claim appeared: README.md's 30-day paragraph, the imgcache package doc, DefaultCacheTTL, EvictionAfterLastCheck, Config.CacheTTL and TODO.md.
Fixed: FileStore.Open returns its fs.FileInfo, and openVariant is removed.
Judgement call: a renewal also changes the ETag, so a client revalidating afterwards downloads the image again.
Model: opus-5-5
Reworked at `445ff62`, on `next` at `b3b8408`.
1. Fixed: the `ETag` is the first 16 characters of the variant key and the variant file's modification time, which the hit already reads, so no database call; test `TestHandleImage_ConditionalRequest_EarlierETagGetsChangedImage`; `README.md` says what it is made from in both places.
2. Fixed in those three lines, and where the same claim appeared: `README.md`'s 30-day paragraph, the `imgcache` package doc, `DefaultCacheTTL`, `EvictionAfterLastCheck`, `Config.CacheTTL` and `TODO.md`.
3. Fixed: `FileStore.Open` returns its `fs.FileInfo`, and `openVariant` is removed.
Judgement call: a renewal also changes the `ETag`, so a client revalidating afterwards downloads the image again.
Model: opus-5-5
The previous review's three findings are fixed, and the change is ready to merge into next at b3b8408.
Open point, not a finding: with the disk cache off (cache_max_bytes set to 0), every response now gets a new ETag, so If-None-Match never gets 304 there; on next it did. Each response is still correct.
Still open for sneak, raised in earlier reviews: images keep immutable and up to one year of max-age, so browsers keep a changed image long after pixa picks up the change.
Model: opus-5-5
Verdict: PASS
Head: 445ff628fd1e55e47c1830a9a9d8e40b91bf510e
The previous review's three findings are fixed, and the change is ready to merge into `next` at `b3b8408`.
- Open point, not a finding: with the disk cache off (`cache_max_bytes` set to 0), every response now gets a new `ETag`, so `If-None-Match` never gets 304 there; on `next` it did. Each response is still correct.
- Still open for sneak, raised in earlier reviews: images keep `immutable` and up to one year of `max-age`, so browsers keep a changed image long after pixa picks up the change.
Model: opus-5-5
For the reworker: the review found the code sound; the only conflict is TODO.md. Rebase onto next and drop every TODO.md change from this PR (take next's TODO.md as is; finished work is not logged there any more). Change nothing else; run make check; push. The PR then merges on a clean rebase.
Model: opus-5-5
For the reworker: the review found the code sound; the only conflict is `TODO.md`. Rebase onto `next` and drop every `TODO.md` change from this PR (take `next`'s `TODO.md` as is; finished work is not logged there any more). Change nothing else; run `make check`; push. The PR then merges on a clean rebase.
Model: opus-5-5
Cached images were served until eviction, so upstream changes were
missed.
After 24 hours a request checks the source with upstream, sending its
stored ETag and Last-Modified. A 304 renews it with no download or
conversion. New bytes replace it; older variants are converted again.
A failed check serves the cached image and puts the URL in the
negative cache. Images not checked for 30 days are evicted.
Variant rows keep the source's ETag and Last-Modified for checking a
variant of an evicted source.
A variant file's modification time is its source's check time when it
was made or renewed. Hits read it without a database call. The ETag
includes it, so new bytes get a new ETag.
Model: opus-5-5
Reworked at bfe3861, rebased onto next at 1379497.
TODO.md is next's as is; this PR no longer changes it. Nothing else changed.
Model: opus-5-5
Reworked at `bfe3861`, rebased onto `next` at `1379497`.
1. `TODO.md` is `next`'s as is; this PR no longer changes it. Nothing else changed.
Model: opus-5-5
clawbot
merged commit e34463acf8 into next2026-11-11 13:37:15 +01:00
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.
Closes #69, as sneak ruled.
What changed:
ETagandLast-Modified.ETagandLast-Modified, so a variant of an evicted source is checked the same way.ETagincludes its file's modification time, so new bytes give a newETag.Not obvious from the diff:
Fetcher.Fetch,Cache.VariantandFileStore.Openchange signature; existing tests change only their calls.TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgo: it pinned the behaviour sneak ruled wrong.Last-Modifiedresponse header.Disclosures:
ETag, so a client revalidating afterwards downloads the image again.immutableand up to one year ofmax-age, so browsers keep their copy past the TTL.Model: opus-5-5
FAIL (
needs-rework), at8feed69on currentnext.internal/imageproxy/imageproxy.go:357(processFromSourceOrFetch): when a cached variant is past the TTL but its source row is gone, the request makes an unconditional fetch. If that fetch fails, the client gets an error instead of the cached variant. This case is common: a cache hit updates only the variant's access time, so size eviction removes the sources of the most-requested variants first. A timeout or a connection error also puts nothing in the negative cache, so every request for that image asks upstream again. That breaks both "on upstream failure, serve the stale version" and the no-hammering rule from #83. The README line "When the check fails, pixa serves the cached image" is false for this case. Acceptable: when any check of a cached variant fails, the cached variant is served and the source URL goes in the negative cache, whether or not the source is still cached. Add tests with the source row and file removed: the variant is served while upstream fails, and upstream is asked once over repeated requests.Last-Modifiedresponse header when upstream sent one) and the matching test from item 5 are not built.TODO.mdstill lists "Last-Modified headers" under Future Steps. Acceptable: build the header and its test, or reference the issue without closing it.internal/imgcache/cache.go:51CacheMaxAge,EvictPastMaxAge(eviction.go:137),max_age_internal_test.goand the log "past the maximum age": "max-age" already names theCache-Controldirective pixa sends on every image (internal/handlers/image.gocacheControl, README). Acceptable: names that say this is how long an entry not checked with upstream is kept before eviction.internal/imgcache/cache.go:1-7: the package doc still says eviction removes the least recently used images to stay under the size limit. It does not mention eviction by age or the TTL check this PR adds to the package. Acceptable: the doc names both.README.md:5: "Both source and transformed images are cached to disk so that subsequent requests are served without origin fetches or additional processing." This is now false, since a request after 24 hours checks upstream. Acceptable: a short sentence that is correct.README.md:237: thevariant_contentitem lists three things in one 28-word sentence, separated by commas. Thesource_contentitem above it was turned into bullets. Acceptable: nested bullets, likesource_content.Model: opus-5-5
8feed69143to049a051596Check cached sources with upstream after the cache TTL (closes #69, closes #83)to Check cached sources with upstream after the cache TTL (closes #69)Reworked at
049a051, onnextat9f1e9f5.fetchAndProcessserves it and puts the URL in the negative cache, whatever the failure; a miss with no cached variant is unchanged. Tests:TestProxy_Get_UpstreamFailureAfterTTLServesVariantOfEvictedSource,TestProxy_Get_FailingUpstreamIsAskedOnceForVariantOfEvictedSource. The README's Cache TTL section covers the case.Last-Modifiedresponse header;TODO.mdkeeps that item.EvictionAfterLastCheck,EvictNotCheckedRecently,evict_not_checked_recently_internal_test.go; the logs say "not checked with upstream recently".Model: opus-5-5
Verdict: FAIL
Head:
049a051596needs-rework, onnextat9f1e9f5.internal/imageproxy/imageproxy.go:497-502(fetchAndProcess, reached fromprocessFromSourceOrFetchwhen the source row is gone): a cached variant past the cache TTL whose source was evicted is checked with an unconditional fetch. An unchanged image is then downloaded and converted again. sneak ruled on #69 to "avoid redownloading if it hasn't changed", and the definition of done says an unchanged image is not downloaded again. This is the usual case once the cache reaches its size limit: a hit renews only the variant's last use, and a 304 renews only the source'schecked_at(RenewSource), so size eviction removes the sources of the most-served variants first. The README's Cache TTL section (README.md:310) documents the download as intended. Acceptable: the source'sETagandLast-Modifiedoutlive the eviction of its source (for example, kept on the variant's row), so the check of such a variant is conditional, and a 304 renews the variant with no download and no conversion. Add a test: source evicted, upstream answers 304, no download and no transform. Correct the README paragraph to match.README.md:316-317and theCache.Variantdoc (internal/imgcache/cache.go:191): "A variant file's modification time is when its source was last checked with upstream" is false. After one variant's check renews the source, each other variant of that source keeps its earlier time. The first request for each is then a miss, renewed from the database without asking upstream. "still" in the next sentence narrates history. Acceptable: a short correct sentence, such as "A variant file's modification time is its source's check time when the variant was made or last renewed", and no "still".Disclosures:
immutablewith amax-ageof up to one year, so browsers and shared caches keep a changed image for up to a year after pixa picks up the change.TestProxy_Get_ServesSourceAndVariantCachedTwoDaysAgoaccepted; it pinned the behaviour sneak ruled wrong, andcache_ttl_internal_test.gocovers the fixed behaviour.Model: opus-5-5
049a051596to880845f38c880845f38cto3f487ee0ceReworked at
3f487ee, rebased ontonextatb3b8408.ETagandLast-Modifiedof the source they were made from, sofetchAndProcesschecks a variant of an evicted source conditionally, and a 304 renews the variant with no download and no conversion. Test: thesource evictedrows ofTestProxy_Get_UnchangedSourceAfterTTLIsNotDownloadedAgain. The README paragraph is corrected.README.mdand theCache.Variantdoc with the suggested sentence, without "still".ErrStale's doc, thevariant_contentitem under Storage andTODO.mdrepeated the claim and are fixed too.Deviation, from the rebase:
nextrewroteREADME.md, so this PR's README changes are re-applied to it, with Cache TTL after Storage. Two new sentences there were false with this PR: the intro's "without a fetch or a conversion", andMISSmeaning only "made for this request". Both now mention the 24 hours.Model: opus-5-5
Verdict: FAIL
Head:
3f487ee0ceneeds-rework, onnextatb3b8408.internal/imageproxy/imageproxy.go:180and:291(formatETag(cacheKey)), answered bynotModifiedininternal/handlers/image.go: theETagcomes only from the variant key. When this PR makes a variant again from new source bytes, theETagstays the same. A browser or shared cache that revalidates withIf-None-Matchthen gets 304 Not Modified and keeps the old image, although pixa serves the new one.README.md:522saysimmutablestops revalidation only while a copy is fresh, so revalidation is how a client would get the change. Acceptable: variants made from different source bytes get differentETags, and a hit still makes no database call.README.md:236-237and:525say what theETagis made from. Add a test: after the source changes upstream, a request that carries the earlierETaggets the new image, not 304.README.md:252,:897and:905: "pixa serves a cached source and its variants" and "pixa serves the cached source and its variants for another 24 hours" are false. pixa serves only variants (README.md:233), andorigis "not the source's bytes" (README.md:715). Acceptable: short sentences that say pixa uses the cached source and serves its variants, for example "pixa uses a cached source for 24 hours after upstream last sent it or confirmed it unchanged."internal/imgcache/cache.go:702-707(openVariant): it drops the size thatFileStore.Openread withStat, then callsfile.Stat()again, so every cache hit now makes twofstatcalls. Acceptable: one stat per open, for exampleFileStore.Openreturning thefs.FileInfoit already has.Disclosure:
immutableand amax-ageof up to one year, so browsers and shared caches keep a changed image for up to a year after pixa picks up the change.Model: opus-5-5
For the reworker (third failed review; this ends the loop): fix exactly the findings of the newest review as its "Acceptable" lines say, nothing else. The next review checks only those.
Model: opus-5-5
3f487ee0ceto445ff628fdReworked at
445ff62, onnextatb3b8408.ETagis the first 16 characters of the variant key and the variant file's modification time, which the hit already reads, so no database call; testTestHandleImage_ConditionalRequest_EarlierETagGetsChangedImage;README.mdsays what it is made from in both places.README.md's 30-day paragraph, theimgcachepackage doc,DefaultCacheTTL,EvictionAfterLastCheck,Config.CacheTTLandTODO.md.FileStore.Openreturns itsfs.FileInfo, andopenVariantis removed.Judgement call: a renewal also changes the
ETag, so a client revalidating afterwards downloads the image again.Model: opus-5-5
Verdict: PASS
Head:
445ff628fdThe previous review's three findings are fixed, and the change is ready to merge into
nextatb3b8408.cache_max_bytesset to 0), every response now gets a newETag, soIf-None-Matchnever gets 304 there; onnextit did. Each response is still correct.immutableand up to one year ofmax-age, so browsers keep a changed image long after pixa picks up the change.Model: opus-5-5
For the reworker: the review found the code sound; the only conflict is
TODO.md. Rebase ontonextand drop everyTODO.mdchange from this PR (takenext'sTODO.mdas is; finished work is not logged there any more). Change nothing else; runmake check; push. The PR then merges on a clean rebase.Model: opus-5-5
445ff628fdtobfe3861314Reworked at
bfe3861, rebased ontonextat1379497.TODO.mdisnext's as is; this PR no longer changes it. Nothing else changed.Model: opus-5-5