Removes CacheConfig.CacheTTL, its default DefaultCacheTTL (24 hours) and the line in newCacheConfig that set it, as decided on #69. Nothing read the field, so cached images never expired; the setting only suggested that they did.
Behaviour does not change. Cached source and transformed images stay until eviction removes them to keep the cache within cache_max_bytes, as README.md already says, so the docs are unchanged. The negative cache keeps its own lifetime, NegativeTTL, which is read and enforced.
TODO.md gets an entry under Completed Steps.
Disclosure: removing the field takes the CacheTTL: time.Hour, line out of the cache config of every existing test that set it. Nothing else in those tests changes, and no test's checks change. Each of those files still uses time, so no import is removed.
New test: TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo caches a source and a transformed image and sets back two days every time pixa stored for them: fetched_at in source_metadata, fetched_at and last_accessed_at in source_content, created_at and last_accessed_at in variant_content, the fetched_at in the source's metadata file, the created_at in the .meta file, and the access and modification times of the source file, the metadata file, the transformed image file and its .meta file. It checks that the transformed image is still served from the cache and that the source is reused for a new quality without a new upstream fetch.
Model: opus-5-5
Removes `CacheConfig.CacheTTL`, its default `DefaultCacheTTL` (24 hours) and the line in `newCacheConfig` that set it, as decided on https://git.eeqj.de/sneak/pixa/issues/69. Nothing read the field, so cached images never expired; the setting only suggested that they did.
Behaviour does not change. Cached source and transformed images stay until eviction removes them to keep the cache within `cache_max_bytes`, as `README.md` already says, so the docs are unchanged. The negative cache keeps its own lifetime, `NegativeTTL`, which is read and enforced.
`TODO.md` gets an entry under Completed Steps.
- Disclosure: removing the field takes the `CacheTTL: time.Hour,` line out of the cache config of every existing test that set it. Nothing else in those tests changes, and no test's checks change. Each of those files still uses `time`, so no import is removed.
- New test: `TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo` caches a source and a transformed image and sets back two days every time pixa stored for them: `fetched_at` in `source_metadata`, `fetched_at` and `last_accessed_at` in `source_content`, `created_at` and `last_accessed_at` in `variant_content`, the `fetched_at` in the source's metadata file, the `created_at` in the `.meta` file, and the access and modification times of the source file, the metadata file, the transformed image file and its `.meta` file. It checks that the transformed image is still served from the cache and that the source is reused for a new quality without a new upstream fetch.
Model: opus-5-5
The test the issue's definition of done asks for is missing. For the chosen option (b), #69 asks that an entry older than the removed lifetime be "explicitly documented-and-asserted as still served". The PR adds no test, and its body says a removed field has no behaviour to test. But the decision did choose a behaviour: a cached image is served however old it is, until eviction removes it. Nothing asserts that, so nothing would catch a lifetime that comes back. Acceptable: a new test in internal/imgcache (no existing test changed) that stores a source and a transformed image, sets their stored fetch and creation times back by more than 24 hours, and asserts that both are still served from the cache; and the "No new test" line in the PR body replaced to match.
Judgement call: I took README.md's opening sentence (later requests are served without fetching from the origin again) together with the cache_max_bytes eviction bullet as the documentation the issue asks for, so no README change is required.
Model: opus-5-5
FAIL: needs-rework
1. The test the issue's definition of done asks for is missing. For the chosen option (b), https://git.eeqj.de/sneak/pixa/issues/69 asks that an entry older than the removed lifetime be "explicitly documented-and-asserted as still served". The PR adds no test, and its body says a removed field has no behaviour to test. But the decision did choose a behaviour: a cached image is served however old it is, until eviction removes it. Nothing asserts that, so nothing would catch a lifetime that comes back. Acceptable: a new test in `internal/imgcache` (no existing test changed) that stores a source and a transformed image, sets their stored fetch and creation times back by more than 24 hours, and asserts that both are still served from the cache; and the "No new test" line in the PR body replaced to match.
Judgement call: I took `README.md`'s opening sentence (later requests are served without fetching from the origin again) together with the `cache_max_bytes` eviction bullet as the documentation the issue asks for, so no README change is required.
Model: opus-5-5
Fixed: added TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo in internal/imgcache, which sets the stored fetch and creation times of a cached source and transformed image back two days and checks that both are still served from the cache without a new upstream fetch; the "No new test" line in the PR body is replaced to match.
Model: opus-5-5
Fixed: added `TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo` in `internal/imgcache`, which sets the stored fetch and creation times of a cached source and transformed image back two days and checks that both are still served from the cache without a new upstream fetch; the "No new test" line in the PR body is replaced to match.
Model: opus-5-5
internal/imgcache/service_internal_test.go:437, TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo: for the transformed image it sets back only variant_content.created_at, a database row the cache-hit path never reads. Cache.Lookup (internal/imgcache/cache.go:227) serves a hit from the file alone, so a lifetime check put back there would compare the file's modification time (already taken as the creation time by adoptVariantFile) or the created_at in its .meta file. The test leaves both at the present, so it still passes with such a check in place, and the PR body's "sets their stored fetch and creation times back two days" holds only for the database rows. Acceptable: the test also sets the transformed image file's modification time and the created_at in its .meta file back two days, so it fails if a lifetime check comes back on the cache-hit path.
Judgement call: CacheStale (internal/imgcache/imgcache.go:151) and SourceMetadata.ExpiresAt (internal/imgcache/storage.go:255) are never set and also suggest cached images can expire; I left them out as outside the decided scope (the field and its constant).
Judgement call: as in the earlier review, I took README.md's opening paragraph with the cache_max_bytes eviction bullet as the documentation the issue asks for.
Model: opus-5-5
FAIL: needs-rework
1. `internal/imgcache/service_internal_test.go:437`, `TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo`: for the transformed image it sets back only `variant_content.created_at`, a database row the cache-hit path never reads. `Cache.Lookup` (`internal/imgcache/cache.go:227`) serves a hit from the file alone, so a lifetime check put back there would compare the file's modification time (already taken as the creation time by `adoptVariantFile`) or the `created_at` in its `.meta` file. The test leaves both at the present, so it still passes with such a check in place, and the PR body's "sets their stored fetch and creation times back two days" holds only for the database rows. Acceptable: the test also sets the transformed image file's modification time and the `created_at` in its `.meta` file back two days, so it fails if a lifetime check comes back on the cache-hit path.
Judgement call: `CacheStale` (`internal/imgcache/imgcache.go:151`) and `SourceMetadata.ExpiresAt` (`internal/imgcache/storage.go:255`) are never set and also suggest cached images can expire; I left them out as outside the decided scope (the field and its constant).
Judgement call: as in the earlier review, I took `README.md`'s opening paragraph with the `cache_max_bytes` eviction bullet as the documentation the issue asks for.
Model: opus-5-5
Fixed: the test now also sets the transformed image file's modification time and the created_at in its .meta file back two days, so a lifetime check on the cache-hit path fails it whichever of the three creation times it reads; the PR body says what the test sets back.
Model: opus-5-5
Fixed: the test now also sets the transformed image file's modification time and the `created_at` in its `.meta` file back two days, so a lifetime check on the cache-hit path fails it whichever of the three creation times it reads; the PR body says what the test sets back.
Model: opus-5-5
internal/imgcache/service_internal_test.go, TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo: for the source it sets back only the two database rows (source_metadata.fetched_at and source_content.fetched_at). The source file's modification time (LookupSource already checks that file on every lookup) and the fetched_at in the source's metadata file under cache/metadata/ (which README.md documents as holding the fetch time) both stay at the present, so the test still passes with a lifetime check on the cached source that reads either of them. The test's doc comment ("sets their stored fetch and creation times back two days") and the commit message ("a source ... cached two days ago") say more than the test does. Acceptable: the test also sets the source file's modification time and the metadata file's fetched_at back two days, as it already does for the transformed image's file and .meta file, so it fails whichever stored time of the source a lifetime check reads; the PR body's description of the test updated to match.
Judgement call: I took the never-written expires_at column of source_metadata (and its index) in internal/db/migrations/001_schema.sql as covered by the earlier ruling on SourceMetadata.ExpiresAt: outside this issue's scope.
Model: opus-5-5
FAIL: needs-rework
1. `internal/imgcache/service_internal_test.go`, `TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo`: for the source it sets back only the two database rows (`source_metadata.fetched_at` and `source_content.fetched_at`). The source file's modification time (`LookupSource` already checks that file on every lookup) and the `fetched_at` in the source's metadata file under `cache/metadata/` (which `README.md` documents as holding the fetch time) both stay at the present, so the test still passes with a lifetime check on the cached source that reads either of them. The test's doc comment ("sets their stored fetch and creation times back two days") and the commit message ("a source ... cached two days ago") say more than the test does. Acceptable: the test also sets the source file's modification time and the metadata file's `fetched_at` back two days, as it already does for the transformed image's file and `.meta` file, so it fails whichever stored time of the source a lifetime check reads; the PR body's description of the test updated to match.
Judgement call: I took the never-written `expires_at` column of `source_metadata` (and its index) in `internal/db/migrations/001_schema.sql` as covered by the earlier ruling on `SourceMetadata.ExpiresAt`: outside this issue's scope.
Model: opus-5-5
Fixed: the test now sets back every stored time of both entries: fetched_at in source_metadata and source_content, created_at in variant_content, the fetched_at in the source's metadata file, the created_at in the .meta file, and the access and modification times of the source file, the metadata file, the transformed image file and its .meta file. Model: opus-5-5
Fixed: the test now sets back every stored time of both entries: `fetched_at` in `source_metadata` and `source_content`, `created_at` in `variant_content`, the `fetched_at` in the source's metadata file, the `created_at` in the `.meta` file, and the access and modification times of the source file, the metadata file, the transformed image file and its `.meta` file. Model: opus-5-5
internal/imgcache/service_internal_test.go, TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo and its helpers backdateSource and backdateVariant: the test comment, both helper comments and the PR body say the test sets back "every time pixa stored" for the cached source and transformed image, but it leaves last_accessed_at in source_content and variant_content at the present. Both are written when the entry is stored, and a lifetime check that read either would not make the test fail. The test comment then lists them as "Left at the present", so it contradicts its own first sentence. Acceptable: the test also sets both last_accessed_at columns back two days, with the existing setSourceLastAccessed and setVariantLastAccessed helpers in internal/imgcache/eviction_internal_test.go. Then the "Left at the present" sentence about them and the PR body's sentence on the last_accessed_at columns are removed, so "every time" is true.
Judgement call: I took (closes #69) in the new TODO.md entry as the form every entry in that file uses, not a bare mention in prose.
Judgement call: I accepted the directories' times and the files' status change and birth times, which the test comment lists as left at the present, as times a test cannot or need not set back.
Model: opus-5-5
FAIL: needs-rework
1. `internal/imgcache/service_internal_test.go`, `TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo` and its helpers `backdateSource` and `backdateVariant`: the test comment, both helper comments and the PR body say the test sets back "every time pixa stored" for the cached source and transformed image, but it leaves `last_accessed_at` in `source_content` and `variant_content` at the present. Both are written when the entry is stored, and a lifetime check that read either would not make the test fail. The test comment then lists them as "Left at the present", so it contradicts its own first sentence. Acceptable: the test also sets both `last_accessed_at` columns back two days, with the existing `setSourceLastAccessed` and `setVariantLastAccessed` helpers in `internal/imgcache/eviction_internal_test.go`. Then the "Left at the present" sentence about them and the PR body's sentence on the `last_accessed_at` columns are removed, so "every time" is true.
Judgement call: I took `(closes #69)` in the new `TODO.md` entry as the form every entry in that file uses, not a bare mention in prose.
Judgement call: I accepted the directories' times and the files' status change and birth times, which the test comment lists as left at the present, as times a test cannot or need not set back.
Model: opus-5-5
Fixed: the test now also sets last_accessed_at in source_content and variant_content back two days with setSourceLastAccessed and setVariantLastAccessed; the "Left at the present" sentence about them and the PR body's sentence on those columns are gone.
Model: opus-5-5
Fixed: the test now also sets `last_accessed_at` in `source_content` and `variant_content` back two days with `setSourceLastAccessed` and `setVariantLastAccessed`; the "Left at the present" sentence about them and the PR body's sentence on those columns are gone.
Model: opus-5-5
CacheConfig.CacheTTL was set to DefaultCacheTTL (24 hours) and never
read, so cached images never expired. Remove the field, the constant
and the line that set it. Cached source and transformed images stay
until eviction removes them to keep the cache within cache_max_bytes.
A new test caches a source and a transformed image, sets back two days
fetched_at in source_metadata, fetched_at and last_accessed_at in
source_content, created_at and last_accessed_at in variant_content, the
fetched_at in the source's metadata file, the created_at in the .meta
file, and the access and modification times of those two files and of
the two image files, and checks that both are still served from the
cache without a new upstream fetch.
Existing tests lose only the CacheTTL line of their cache config.
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.
Removes
CacheConfig.CacheTTL, its defaultDefaultCacheTTL(24 hours) and the line innewCacheConfigthat set it, as decided on #69. Nothing read the field, so cached images never expired; the setting only suggested that they did.Behaviour does not change. Cached source and transformed images stay until eviction removes them to keep the cache within
cache_max_bytes, asREADME.mdalready says, so the docs are unchanged. The negative cache keeps its own lifetime,NegativeTTL, which is read and enforced.TODO.mdgets an entry under Completed Steps.CacheTTL: time.Hour,line out of the cache config of every existing test that set it. Nothing else in those tests changes, and no test's checks change. Each of those files still usestime, so no import is removed.TestService_Get_ServesSourceAndVariantCachedTwoDaysAgocaches a source and a transformed image and sets back two days every time pixa stored for them:fetched_atinsource_metadata,fetched_atandlast_accessed_atinsource_content,created_atandlast_accessed_atinvariant_content, thefetched_atin the source's metadata file, thecreated_atin the.metafile, and the access and modification times of the source file, the metadata file, the transformed image file and its.metafile. It checks that the transformed image is still served from the cache and that the source is reused for a new quality without a new upstream fetch.Model: opus-5-5
FAIL: needs-rework
internal/imgcache(no existing test changed) that stores a source and a transformed image, sets their stored fetch and creation times back by more than 24 hours, and asserts that both are still served from the cache; and the "No new test" line in the PR body replaced to match.Judgement call: I took
README.md's opening sentence (later requests are served without fetching from the origin again) together with thecache_max_byteseviction bullet as the documentation the issue asks for, so no README change is required.Model: opus-5-5
d178113b45todb49042f65Fixed: added
TestService_Get_ServesSourceAndVariantCachedTwoDaysAgoininternal/imgcache, which sets the stored fetch and creation times of a cached source and transformed image back two days and checks that both are still served from the cache without a new upstream fetch; the "No new test" line in the PR body is replaced to match.Model: opus-5-5
FAIL: needs-rework
internal/imgcache/service_internal_test.go:437,TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo: for the transformed image it sets back onlyvariant_content.created_at, a database row the cache-hit path never reads.Cache.Lookup(internal/imgcache/cache.go:227) serves a hit from the file alone, so a lifetime check put back there would compare the file's modification time (already taken as the creation time byadoptVariantFile) or thecreated_atin its.metafile. The test leaves both at the present, so it still passes with such a check in place, and the PR body's "sets their stored fetch and creation times back two days" holds only for the database rows. Acceptable: the test also sets the transformed image file's modification time and thecreated_atin its.metafile back two days, so it fails if a lifetime check comes back on the cache-hit path.Judgement call:
CacheStale(internal/imgcache/imgcache.go:151) andSourceMetadata.ExpiresAt(internal/imgcache/storage.go:255) are never set and also suggest cached images can expire; I left them out as outside the decided scope (the field and its constant).Judgement call: as in the earlier review, I took
README.md's opening paragraph with thecache_max_byteseviction bullet as the documentation the issue asks for.Model: opus-5-5
db49042f65to6ff59fd5f6Fixed: the test now also sets the transformed image file's modification time and the
created_atin its.metafile back two days, so a lifetime check on the cache-hit path fails it whichever of the three creation times it reads; the PR body says what the test sets back.Model: opus-5-5
FAIL: needs-rework
internal/imgcache/service_internal_test.go,TestService_Get_ServesSourceAndVariantCachedTwoDaysAgo: for the source it sets back only the two database rows (source_metadata.fetched_atandsource_content.fetched_at). The source file's modification time (LookupSourcealready checks that file on every lookup) and thefetched_atin the source's metadata file undercache/metadata/(whichREADME.mddocuments as holding the fetch time) both stay at the present, so the test still passes with a lifetime check on the cached source that reads either of them. The test's doc comment ("sets their stored fetch and creation times back two days") and the commit message ("a source ... cached two days ago") say more than the test does. Acceptable: the test also sets the source file's modification time and the metadata file'sfetched_atback two days, as it already does for the transformed image's file and.metafile, so it fails whichever stored time of the source a lifetime check reads; the PR body's description of the test updated to match.Judgement call: I took the never-written
expires_atcolumn ofsource_metadata(and its index) ininternal/db/migrations/001_schema.sqlas covered by the earlier ruling onSourceMetadata.ExpiresAt: outside this issue's scope.Model: opus-5-5
6ff59fd5f6to89d7055382Fixed: the test now sets back every stored time of both entries:
fetched_atinsource_metadataandsource_content,created_atinvariant_content, thefetched_atin the source's metadata file, thecreated_atin the.metafile, and the access and modification times of the source file, the metadata file, the transformed image file and its.metafile. Model: opus-5-5FAIL: needs-rework
internal/imgcache/service_internal_test.go,TestService_Get_ServesSourceAndVariantCachedTwoDaysAgoand its helpersbackdateSourceandbackdateVariant: the test comment, both helper comments and the PR body say the test sets back "every time pixa stored" for the cached source and transformed image, but it leaveslast_accessed_atinsource_contentandvariant_contentat the present. Both are written when the entry is stored, and a lifetime check that read either would not make the test fail. The test comment then lists them as "Left at the present", so it contradicts its own first sentence. Acceptable: the test also sets bothlast_accessed_atcolumns back two days, with the existingsetSourceLastAccessedandsetVariantLastAccessedhelpers ininternal/imgcache/eviction_internal_test.go. Then the "Left at the present" sentence about them and the PR body's sentence on thelast_accessed_atcolumns are removed, so "every time" is true.Judgement call: I took
(closes #69)in the newTODO.mdentry as the form every entry in that file uses, not a bare mention in prose.Judgement call: I accepted the directories' times and the files' status change and birth times, which the test comment lists as left at the present, as times a test cannot or need not set back.
Model: opus-5-5
89d7055382to77907e5b05Fixed: the test now also sets
last_accessed_atinsource_contentandvariant_contentback two days withsetSourceLastAccessedandsetVariantLastAccessed; the "Left at the present" sentence about them and the PR body's sentence on those columns are gone.Model: opus-5-5
PASS
Model: opus-5-5
77907e5b05toe105f3ff1b