Cache.metaCache was declared but unused, so every cache hit read and parsed the variant's .meta file. It is now an LRU (github.com/hashicorp/golang-lru/v2) of up to 10,000 variants' content types, filled by StoreVariant and by GetVariant after it reads a .meta file; for a variant it holds, GetVariant skips that read.
What the diff does not show:
Only the content type is kept. The variant file is still opened and its size taken from it, so a file rewritten by a concurrent store is never sent with an old Content-Length.
Nothing is served from memory alone: Lookup still checks that the file exists, eviction removes the entry before deleting the files, and GetVariant removes it when the file will not open.
Memory only ever holds a type read from a .meta file or passed to StoreVariant. VariantStorage.LoadWithMeta returns no type when the .meta file is missing, as partway through a store, and GetVariant then serves application/octet-stream without keeping it. A type read from disk is added only when memory has none, so the store's wins.
Disclosures:
Deviation: the second-hit test checks the type survives deleting the .meta file; it counts no reads.
Judgement call: the part of GetVariant after its check of memory is its own method, loadVariantWithMeta, so a test can run it after a store.
Judgement call: the cap is a constant, not a setting.
Judgement call: README.md keeps the 1-5k r/s target as an aim; nothing measures it.
The go.mod and go.sum lines were written by hand from the Go checksum database; no make target adds a dependency.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/pixa/issues/70.
`Cache.metaCache` was declared but unused, so every cache hit read and parsed the variant's `.meta` file. It is now an LRU (`github.com/hashicorp/golang-lru/v2`) of up to 10,000 variants' content types, filled by `StoreVariant` and by `GetVariant` after it reads a `.meta` file; for a variant it holds, `GetVariant` skips that read.
What the diff does not show:
- Only the content type is kept. The variant file is still opened and its size taken from it, so a file rewritten by a concurrent store is never sent with an old `Content-Length`.
- Nothing is served from memory alone: `Lookup` still checks that the file exists, eviction removes the entry before deleting the files, and `GetVariant` removes it when the file will not open.
- Memory only ever holds a type read from a `.meta` file or passed to `StoreVariant`. `VariantStorage.LoadWithMeta` returns no type when the `.meta` file is missing, as partway through a store, and `GetVariant` then serves `application/octet-stream` without keeping it. A type read from disk is added only when memory has none, so the store's wins.
Disclosures:
- Deviation: the second-hit test checks the type survives deleting the `.meta` file; it counts no reads.
- Judgement call: the part of `GetVariant` after its check of memory is its own method, `loadVariantWithMeta`, so a test can run it after a store.
- Judgement call: the cap is a constant, not a setting.
- Judgement call: `README.md` keeps the 1-5k r/s target as an aim; nothing measures it.
- The `go.mod` and `go.sum` lines were written by hand from the Go checksum database; no make target adds a dependency.
Model: opus-5-5
internal/imgcache/cache.go:213 (GetVariant): the content type read from a .meta file always overwrites whatever memory holds for that variant. When a request reads a new variant after its file appears but before its .meta file is written, it gets application/octet-stream. If that request writes to memory after StoreVariant has, the wrong type stays in memory, and every later hit serves the image with it until the entry drops out, which for a popular image means until restart. Before this change the wrong type affected only that one request. Acceptable: GetVariant adds a content type only when memory has none for the variant (for example with ContainsOrAdd), so the type recorded by a store always wins.
internal/imgcache/cache.go:170 (Lookup): no test covers Lookup treating a variant held in memory as present without checking the disk. Acceptable: a test that fails without that check, or remove the check from Lookup, since GetVariant opens the file anyway.
Model: opus-5-5
FAIL
1. `internal/imgcache/cache.go:213` (`GetVariant`): the content type read from a `.meta` file always overwrites whatever memory holds for that variant. When a request reads a new variant after its file appears but before its `.meta` file is written, it gets `application/octet-stream`. If that request writes to memory after `StoreVariant` has, the wrong type stays in memory, and every later hit serves the image with it until the entry drops out, which for a popular image means until restart. Before this change the wrong type affected only that one request. Acceptable: `GetVariant` adds a content type only when memory has none for the variant (for example with `ContainsOrAdd`), so the type recorded by a store always wins.
2. `internal/imgcache/cache.go:170` (`Lookup`): no test covers `Lookup` treating a variant held in memory as present without checking the disk. Acceptable: a test that fails without that check, or remove the check from `Lookup`, since `GetVariant` opens the file anyway.
Model: opus-5-5
GetVariant now adds with ContainsOrAdd; TestReadDuringStoreKeepsStoredContentType fails without it.
Removed the check from Lookup; it looks only at the disk again.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/pixa/pulls/157#issuecomment-106258, rebased onto `next`:
1. `GetVariant` now adds with `ContainsOrAdd`; `TestReadDuringStoreKeepsStoredContentType` fails without it.
2. Removed the check from `Lookup`; it looks only at the disk again.
Model: opus-5-5
internal/imgcache/cache.go, GetVariant and loadVariantWithMeta: the type StoreVariant records can still be replaced by application/octet-stream for good. A read that opens a newly stored variant before its .meta file is written holds application/octet-stream. If another read, which found the entry but could not open the file (evicted just before the store replaced it, or out of file descriptors), removes the entry after the store added it, the first read's ContainsOrAdd finds nothing and keeps application/octet-stream, and every later hit serves the image with it until restart. Acceptable: memory only ever receives a type read from a .meta file or passed to StoreVariant, never the application/octet-stream fallback (or GetVariant no longer removes the entry when the file will not open), with a test that fails without the change by running the store, the failed read and the read without a .meta file in that order; TODO.md and the loadVariantWithMeta comment then stay true.
Model: opus-5-5
FAIL
1. `internal/imgcache/cache.go`, `GetVariant` and `loadVariantWithMeta`: the type `StoreVariant` records can still be replaced by `application/octet-stream` for good. A read that opens a newly stored variant before its `.meta` file is written holds `application/octet-stream`. If another read, which found the entry but could not open the file (evicted just before the store replaced it, or out of file descriptors), removes the entry after the store added it, the first read's `ContainsOrAdd` finds nothing and keeps `application/octet-stream`, and every later hit serves the image with it until restart. Acceptable: memory only ever receives a type read from a `.meta` file or passed to `StoreVariant`, never the `application/octet-stream` fallback (or `GetVariant` no longer removes the entry when the file will not open), with a test that fails without the change by running the store, the failed read and the read without a `.meta` file in that order; `TODO.md` and the `loadVariantWithMeta` comment then stay true.
Model: opus-5-5
Memory now only ever receives a content type read from a .meta file or passed to StoreVariant: without a .meta file GetVariant serves application/octet-stream and keeps nothing. TestFailedReadDuringStoreKeepsStoredContentType runs the store, the failed read and the read without a .meta file in that order and fails without the change; TODO.md and the loadVariantWithMeta comment say the same.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/pixa/pulls/157#issuecomment-106464, rebased onto `next`:
1. Memory now only ever receives a content type read from a `.meta` file or passed to `StoreVariant`: without a `.meta` file `GetVariant` serves `application/octet-stream` and keeps nothing. `TestFailedReadDuringStoreKeepsStoredContentType` runs the store, the failed read and the read without a `.meta` file in that order and fails without the change; `TODO.md` and the `loadVariantWithMeta` comment say the same.
Model: opus-5-5
TODO.md: the branch no longer merges into current next; its Completed Steps entry conflicts with the one added for #71. Acceptable: rebased onto current next, keeping both entries.
internal/imgcache/cache.go, loadVariantWithMeta: no test covers what its comment and TODO.md promise, that a content type StoreVariant put in memory is kept over an older one read from a .meta file. TestReadDuringStoreKeepsStoredContentType was written for this, but since its read now finds no .meta file and keeps nothing, it no longer reaches the ContainsOrAdd call, and replacing that call with Add goes unnoticed. Acceptable: a test in which the .meta file holds a different content type from the one the store put in memory, as a .meta file not yet rewritten by a store would, and a read through loadVariantWithMeta leaves the stored type; the test fails if ContainsOrAdd is replaced with Add.
Model: opus-5-5
FAIL
1. `TODO.md`: the branch no longer merges into current `next`; its Completed Steps entry conflicts with the one added for https://git.eeqj.de/sneak/pixa/issues/71. Acceptable: rebased onto current `next`, keeping both entries.
2. `internal/imgcache/cache.go`, `loadVariantWithMeta`: no test covers what its comment and `TODO.md` promise, that a content type `StoreVariant` put in memory is kept over an older one read from a `.meta` file. `TestReadDuringStoreKeepsStoredContentType` was written for this, but since its read now finds no `.meta` file and keeps nothing, it no longer reaches the `ContainsOrAdd` call, and replacing that call with `Add` goes unnoticed. Acceptable: a test in which the `.meta` file holds a different content type from the one the store put in memory, as a `.meta` file not yet rewritten by a store would, and a read through `loadVariantWithMeta` leaves the stored type; the test fails if `ContainsOrAdd` is replaced with `Add`.
Model: opus-5-5
Tests for keeping each variant's content type in memory. A second hit
must still get the stored content type after the variant's .meta file
is deleted, whether the first came from storing the variant or from
reading it after a restart; this fails now. A variant removed by
EvictToLimit, or whose file was deleted from disk, must be a miss and
must not be served, and concurrent stores, reads and evictions run
under the race detector; these pass now and guard the change.
Model: opus-5-5
Cache.metaCache was declared and never used, so every hit read and
parsed the variant's .meta file. It is now an LRU of up to 10,000
content types (hashicorp/golang-lru/v2), filled by StoreVariant and by
GetVariant after it reads a .meta file. For a variant it holds, Lookup
skips the disk check and GetVariant skips the .meta read; the variant
file is still opened and its size taken from it. Eviction removes the
entry before deleting the files, and GetVariant removes it when the
file will not open, so a missing variant is never served. The cap is a
constant, not a setting. README.md describes it.
Model: opus-5-5
A GetVariant that begins before StoreVariant finishes can find the
variant file but not yet its .meta file, and so reads
application/octet-stream. When it adds that to memory after the store
added the real type, the wrong type is served to every later hit. The
part of GetVariant that runs after its check of memory moves, unchanged,
into loadVariantWithMeta, so the test can run it after a store. The
test fails now.
Model: opus-5-5
GetVariant now adds the content type it read from a .meta file only
when memory holds none for the variant, so a type StoreVariant added
meanwhile is not replaced. A read that found no .meta file yet still
gets application/octet-stream for that one request, as before this
change, but no longer leaves it in memory for later hits.
Model: opus-5-5
Lookup again counts a variant as present only when its file exists.
Treating a variant held in memory as present saved one check of the
disk, but no test covered it, and GetVariant opens the file anyway.
Model: opus-5-5
A read that finds a variant in memory but cannot open its file removes
it from memory. When that happens after StoreVariant has added the
variant's content type, a read that opened the variant file before the
store wrote its .meta file finds memory empty and keeps
application/octet-stream there, so every later hit serves the image
with it. The test runs the store, the failed read and the read without
a .meta file in that order, then checks a later hit. It fails now.
Model: opus-5-5
VariantStorage.LoadWithMeta now returns an empty content type when the
.meta file is missing or unreadable, and GetVariant serves
application/octet-stream in that case without keeping it in memory.
Memory now only ever holds a content type read from a .meta file or
passed to StoreVariant, so a read that ran before a store wrote the
.meta file can no longer leave the wrong type in memory, however it
interleaves with the store, other reads and eviction.
Model: opus-5-5
A read that finds no .meta file keeps nothing in memory, so the
existing test no longer reaches the step that keeps a type StoreVariant
put there. This one writes a .meta file with a different content type
after the store, as one not yet rewritten by it would hold, and checks
that the read leaves the stored type in memory.
Model: opus-5-5
Rebased onto current next; TODO.md keeps both Completed Steps entries, this one on top.
Added TestReadOfOlderMetaFileKeepsStoredContentType: after the store it writes a .meta file holding image/jpeg, reads through loadVariantWithMeta, and checks memory still holds the stored image/webp; with ContainsOrAdd swapped for Add in a scratch copy, it fails.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/pixa/pulls/157#issuecomment-106760:
1. Rebased onto current `next`; `TODO.md` keeps both Completed Steps entries, this one on top.
2. Added `TestReadOfOlderMetaFileKeepsStoredContentType`: after the store it writes a `.meta` file holding `image/jpeg`, reads through `loadVariantWithMeta`, and checks memory still holds the stored `image/webp`; with `ContainsOrAdd` swapped for `Add` in a scratch copy, it fails.
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.
Closes #70.
Cache.metaCachewas declared but unused, so every cache hit read and parsed the variant's.metafile. It is now an LRU (github.com/hashicorp/golang-lru/v2) of up to 10,000 variants' content types, filled byStoreVariantand byGetVariantafter it reads a.metafile; for a variant it holds,GetVariantskips that read.What the diff does not show:
Content-Length.Lookupstill checks that the file exists, eviction removes the entry before deleting the files, andGetVariantremoves it when the file will not open..metafile or passed toStoreVariant.VariantStorage.LoadWithMetareturns no type when the.metafile is missing, as partway through a store, andGetVariantthen servesapplication/octet-streamwithout keeping it. A type read from disk is added only when memory has none, so the store's wins.Disclosures:
.metafile; it counts no reads.GetVariantafter its check of memory is its own method,loadVariantWithMeta, so a test can run it after a store.README.mdkeeps the 1-5k r/s target as an aim; nothing measures it.go.modandgo.sumlines were written by hand from the Go checksum database; no make target adds a dependency.Model: opus-5-5
FAIL
internal/imgcache/cache.go:213(GetVariant): the content type read from a.metafile always overwrites whatever memory holds for that variant. When a request reads a new variant after its file appears but before its.metafile is written, it getsapplication/octet-stream. If that request writes to memory afterStoreVarianthas, the wrong type stays in memory, and every later hit serves the image with it until the entry drops out, which for a popular image means until restart. Before this change the wrong type affected only that one request. Acceptable:GetVariantadds a content type only when memory has none for the variant (for example withContainsOrAdd), so the type recorded by a store always wins.internal/imgcache/cache.go:170(Lookup): no test coversLookuptreating a variant held in memory as present without checking the disk. Acceptable: a test that fails without that check, or remove the check fromLookup, sinceGetVariantopens the file anyway.Model: opus-5-5
85161353eatofc87c2117dRework for #157 (comment), rebased onto
next:GetVariantnow adds withContainsOrAdd;TestReadDuringStoreKeepsStoredContentTypefails without it.Lookup; it looks only at the disk again.Model: opus-5-5
FAIL
internal/imgcache/cache.go,GetVariantandloadVariantWithMeta: the typeStoreVariantrecords can still be replaced byapplication/octet-streamfor good. A read that opens a newly stored variant before its.metafile is written holdsapplication/octet-stream. If another read, which found the entry but could not open the file (evicted just before the store replaced it, or out of file descriptors), removes the entry after the store added it, the first read'sContainsOrAddfinds nothing and keepsapplication/octet-stream, and every later hit serves the image with it until restart. Acceptable: memory only ever receives a type read from a.metafile or passed toStoreVariant, never theapplication/octet-streamfallback (orGetVariantno longer removes the entry when the file will not open), with a test that fails without the change by running the store, the failed read and the read without a.metafile in that order;TODO.mdand theloadVariantWithMetacomment then stay true.Model: opus-5-5
de6f5e34eatoe29257fd42Rework for #157 (comment), rebased onto
next:.metafile or passed toStoreVariant: without a.metafileGetVariantservesapplication/octet-streamand keeps nothing.TestFailedReadDuringStoreKeepsStoredContentTyperuns the store, the failed read and the read without a.metafile in that order and fails without the change;TODO.mdand theloadVariantWithMetacomment say the same.Model: opus-5-5
FAIL
TODO.md: the branch no longer merges into currentnext; its Completed Steps entry conflicts with the one added for #71. Acceptable: rebased onto currentnext, keeping both entries.internal/imgcache/cache.go,loadVariantWithMeta: no test covers what its comment andTODO.mdpromise, that a content typeStoreVariantput in memory is kept over an older one read from a.metafile.TestReadDuringStoreKeepsStoredContentTypewas written for this, but since its read now finds no.metafile and keeps nothing, it no longer reaches theContainsOrAddcall, and replacing that call withAddgoes unnoticed. Acceptable: a test in which the.metafile holds a different content type from the one the store put in memory, as a.metafile not yet rewritten by a store would, and a read throughloadVariantWithMetaleaves the stored type; the test fails ifContainsOrAddis replaced withAdd.Model: opus-5-5
e29257fd42toc6e66bf45aRework for #157 (comment):
next;TODO.mdkeeps both Completed Steps entries, this one on top.TestReadOfOlderMetaFileKeepsStoredContentType: after the store it writes a.metafile holdingimage/jpeg, reads throughloadVariantWithMeta, and checks memory still holds the storedimage/webp; withContainsOrAddswapped forAddin a scratch copy, it fails.Model: opus-5-5
PASS
Model: opus-5-5