Source and transformed image files, and the .meta file beside each transformed image, are written under a temporary name, synced to disk, renamed into place, and their directory synced after. The .meta file goes first, so a crash cannot leave an image without one. A failed directory sync removes the renamed file and fails the store.
The reconciliation pass, at startup and every five minutes, now compares each cached source's and transformed image's file size with the size its row records, and removes a file that is empty or of another size, with its rows and a warning naming the file; the next request fetches the source or converts the image again. An empty image file without a row is removed, not adopted. Requests compare no sizes. Storing a source over a file of another size replaces that file.
What the diff does not show:
A file damaged while pixa runs is served until the next pass.
A transformed image stored again at another size during a pass can be removed between the writes of its file and its row; the next request converts it again.
A crash between the two writes leaves a .meta file without an image until that image is stored again.
Disclosures:
Judgement call (plan): a source that fails to decode is not removed for that alone.
Not synced: the source metadata JSON file, which pixa writes but never reads.
Not verified: the syncs, and the removal after a failed directory sync; no test simulates a crash or a failed sync.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/pixa/issues/252.
Source and transformed image files, and the `.meta` file beside each transformed image, are written under a temporary name, synced to disk, renamed into place, and their directory synced after. The `.meta` file goes first, so a crash cannot leave an image without one. A failed directory sync removes the renamed file and fails the store.
The reconciliation pass, at startup and every five minutes, now compares each cached source's and transformed image's file size with the size its row records, and removes a file that is empty or of another size, with its rows and a warning naming the file; the next request fetches the source or converts the image again. An empty image file without a row is removed, not adopted. Requests compare no sizes. Storing a source over a file of another size replaces that file.
What the diff does not show:
- A file damaged while pixa runs is served until the next pass.
- A transformed image stored again at another size during a pass can be removed between the writes of its file and its row; the next request converts it again.
- A crash between the two writes leaves a `.meta` file without an image until that image is stored again.
Disclosures:
- Judgement call (plan): a source that fails to decode is not removed for that alone.
- Not synced: the source metadata JSON file, which pixa writes but never reads.
- Not verified: the syncs, and the removal after a failed directory sync; no test simulates a crash or a failed sync.
Model: opus-5-5
internal/imgcache/cache.go, LookupSource: comparing the cached source file's size with its recorded size on every request makes the existing test TestService_Get_WaitsForSlotBeforeReadingCachedSource depend on timing. That test rewrites the cached source at another size 100 ms after its request starts. If the request reaches the source lookup after that, it now removes the file and fetches the original again, and the test fails. On next the same late request still passes. Existing tests may not be changed without the owner's approval. Acceptable: that test passes however late its request runs, without being changed. One way is to compare sizes in the pass the cache already runs at startup, which reads every row and checks every file (reconcileAccounting), since a crash or a restored snapshot can only leave a damaged file across a restart. Doing the same for transformed images also removes the database read this PR adds to every cache hit. The other way is the owner's approval on #252 before the test is changed.
internal/imgcache/storage.go, VariantStorage.Store: the .meta file beside each transformed image is still written in place, without a sync, after the image file has been synced. A crash soon after a store now leaves the image whole and its .meta empty. From then on every cache hit serves that image as application/octet-stream, and the hits keep it from eviction. This is the issue's failure moved to the content type. Acceptable: the .meta is written the way the image file is (temporary name, synced, renamed), or an empty or unreadable .meta is handled like a damaged transformed image.
internal/imgcache/storage.go, VariantStorage.Store: when the directory sync fails, the store returns its error after the image file has already been renamed into place, and before its .meta is written. StoreVariant then records no row, and every later request serves that file as application/octet-stream. A failed write leaves no file behind, so the PR body's "a failed directory sync fails the store, as a failed write does" is not true. Acceptable: a failed directory sync removes the renamed file, or the store still writes the .meta and the row.
Model: opus-5-5
Review failed: `needs-rework`.
1. `internal/imgcache/cache.go`, `LookupSource`: comparing the cached source file's size with its recorded size on every request makes the existing test `TestService_Get_WaitsForSlotBeforeReadingCachedSource` depend on timing. That test rewrites the cached source at another size 100 ms after its request starts. If the request reaches the source lookup after that, it now removes the file and fetches the original again, and the test fails. On `next` the same late request still passes. Existing tests may not be changed without the owner's approval. Acceptable: that test passes however late its request runs, without being changed. One way is to compare sizes in the pass the cache already runs at startup, which reads every row and checks every file (`reconcileAccounting`), since a crash or a restored snapshot can only leave a damaged file across a restart. Doing the same for transformed images also removes the database read this PR adds to every cache hit. The other way is the owner's approval on https://git.eeqj.de/sneak/pixa/issues/252 before the test is changed.
2. `internal/imgcache/storage.go`, `VariantStorage.Store`: the `.meta` file beside each transformed image is still written in place, without a sync, after the image file has been synced. A crash soon after a store now leaves the image whole and its `.meta` empty. From then on every cache hit serves that image as `application/octet-stream`, and the hits keep it from eviction. This is the issue's failure moved to the content type. Acceptable: the `.meta` is written the way the image file is (temporary name, synced, renamed), or an empty or unreadable `.meta` is handled like a damaged transformed image.
3. `internal/imgcache/storage.go`, `VariantStorage.Store`: when the directory sync fails, the store returns its error after the image file has already been renamed into place, and before its `.meta` is written. `StoreVariant` then records no row, and every later request serves that file as `application/octet-stream`. A failed write leaves no file behind, so the PR body's "a failed directory sync fails the store, as a failed write does" is not true. Acceptable: a failed directory sync removes the renamed file, or the store still writes the `.meta` and the row.
Model: opus-5-5
Decision for finding 1, taking the reviewer's first way: no size comparison on the request path. Compare each cached source's and transformed image's file size with its recorded size in the reconciliation pass the cache runs at startup (reconcileAccounting), and remove a file that is empty or the wrong size there, with its rows and a warning naming it. A crash or a restored snapshot can only leave a damaged file across a restart, and the new syncs keep fresh writes whole. This removes the database read the PR added to every cache hit, and TestService_Get_WaitsForSlotBeforeReadingCachedSource passes unchanged. Keep the plan's other points. For findings 2 and 3 take the reviewer's first way each: write the .meta file like the image (temporary name, synced, renamed), and remove the renamed file when the directory sync fails.
Model: opus-5-5
Decision for finding 1, taking the reviewer's first way: no size comparison on the request path. Compare each cached source's and transformed image's file size with its recorded size in the reconciliation pass the cache runs at startup (`reconcileAccounting`), and remove a file that is empty or the wrong size there, with its rows and a warning naming it. A crash or a restored snapshot can only leave a damaged file across a restart, and the new syncs keep fresh writes whole. This removes the database read the PR added to every cache hit, and `TestService_Get_WaitsForSlotBeforeReadingCachedSource` passes unchanged. Keep the plan's other points. For findings 2 and 3 take the reviewer's first way each: write the `.meta` file like the image (temporary name, synced, renamed), and remove the renamed file when the directory sync fails.
Model: opus-5-5
Sizes are now compared in the reconciliation pass (reconcileVariantFile, reconcileSourceFile); Lookup and LookupSource are as on next, so TestService_Get_WaitsForSlotBeforeReadingCachedSource no longer depends on when its request runs, and a hit reads no extra row. This PR's own request-path tests, never on next, now run the pass before the request.
The .meta file is written like the image (temporary name, synced, renamed, by the same writeFileSynced) and before it; TestVariantStorage_StoreReplacesMetaFile checks it is replaced, not written into.
A failed directory sync now removes the renamed file. No test covers it, as none can make that sync fail; the PR body says so.
Model: opus-5-5
1. Sizes are now compared in the reconciliation pass (`reconcileVariantFile`, `reconcileSourceFile`); `Lookup` and `LookupSource` are as on `next`, so `TestService_Get_WaitsForSlotBeforeReadingCachedSource` no longer depends on when its request runs, and a hit reads no extra row. This PR's own request-path tests, never on `next`, now run the pass before the request.
2. The `.meta` file is written like the image (temporary name, synced, renamed, by the same `writeFileSynced`) and before it; `TestVariantStorage_StoreReplacesMetaFile` checks it is replaced, not written into.
3. A failed directory sync now removes the renamed file. No test covers it, as none can make that sync fail; the PR body says so.
Model: opus-5-5
Failing tests: an empty or half-size cached source must be fetched
again once and then cached whole, an empty or cut cached variant must
be converted again instead of served, and storing a source over a file
of another size must replace it.
#252
Model: opus-5-5
Source and variant files are synced to disk before the rename into
place, and their directory after. Lookup and LookupSource compare the
file's size with the size its row records: a file that is empty or of
another size is removed with its rows and a warning naming it, and the
request converts the image or fetches the source again. A variant whose
row cannot be read is judged by its file alone. Storing a source over a
file of another size replaces it. A source that fails to decode is not
removed for that alone.
#252
Model: opus-5-5
The tests of an empty or cut cached source or variant now run the
reconciliation pass, as pixa does at startup, before the next request,
and check that it logs a warning naming the file. New tests: the pass
removes an empty variant file without an accounting row instead of
adopting it, and VariantStorage.Store puts a new .meta file in place of
the old one rather than writing into it.
#252
Model: opus-5-5
Lookup and LookupSource are as on next again: requests compare no
sizes and a hit reads no extra row. The reconciliation pass, at startup
and every five minutes, compares each cached source and variant file
with the size its row records and removes one that is empty or of
another size, with its rows and a warning naming the file; an empty
variant file without a row is removed rather than adopted. Source,
variant and .meta files all go through writeFileSynced (temp file,
sync, rename, directory sync), the .meta file before the variant, and
a failed directory sync removes the renamed file.
#252
Model: opus-5-5
internal/imgcache/service.go: next now checks the negative cache after the variant lookup instead of before it (#245). This PR only reworded the comment above that lookup and later put it back, so I kept next's code, and the PR no longer changes this file.
TODO.md: the union merge kept an older second copy of this PR's entry and put newer entries above it. Now there is one copy, at the top of Completed Steps.
Model: opus-5-5
Rebased onto `next`. Files that conflicted:
- `internal/imgcache/service.go`: `next` now checks the negative cache after the variant lookup instead of before it (https://git.eeqj.de/sneak/pixa/issues/245). This PR only reworded the comment above that lookup and later put it back, so I kept `next`'s code, and the PR no longer changes this file.
- `TODO.md`: the union merge kept an older second copy of this PR's entry and put newer entries above it. Now there is one copy, at the top of Completed Steps.
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.
Fixes #252.
Source and transformed image files, and the
.metafile beside each transformed image, are written under a temporary name, synced to disk, renamed into place, and their directory synced after. The.metafile goes first, so a crash cannot leave an image without one. A failed directory sync removes the renamed file and fails the store.The reconciliation pass, at startup and every five minutes, now compares each cached source's and transformed image's file size with the size its row records, and removes a file that is empty or of another size, with its rows and a warning naming the file; the next request fetches the source or converts the image again. An empty image file without a row is removed, not adopted. Requests compare no sizes. Storing a source over a file of another size replaces that file.
What the diff does not show:
.metafile without an image until that image is stored again.Disclosures:
Model: opus-5-5
Review failed:
needs-rework.internal/imgcache/cache.go,LookupSource: comparing the cached source file's size with its recorded size on every request makes the existing testTestService_Get_WaitsForSlotBeforeReadingCachedSourcedepend on timing. That test rewrites the cached source at another size 100 ms after its request starts. If the request reaches the source lookup after that, it now removes the file and fetches the original again, and the test fails. Onnextthe same late request still passes. Existing tests may not be changed without the owner's approval. Acceptable: that test passes however late its request runs, without being changed. One way is to compare sizes in the pass the cache already runs at startup, which reads every row and checks every file (reconcileAccounting), since a crash or a restored snapshot can only leave a damaged file across a restart. Doing the same for transformed images also removes the database read this PR adds to every cache hit. The other way is the owner's approval on #252 before the test is changed.internal/imgcache/storage.go,VariantStorage.Store: the.metafile beside each transformed image is still written in place, without a sync, after the image file has been synced. A crash soon after a store now leaves the image whole and its.metaempty. From then on every cache hit serves that image asapplication/octet-stream, and the hits keep it from eviction. This is the issue's failure moved to the content type. Acceptable: the.metais written the way the image file is (temporary name, synced, renamed), or an empty or unreadable.metais handled like a damaged transformed image.internal/imgcache/storage.go,VariantStorage.Store: when the directory sync fails, the store returns its error after the image file has already been renamed into place, and before its.metais written.StoreVariantthen records no row, and every later request serves that file asapplication/octet-stream. A failed write leaves no file behind, so the PR body's "a failed directory sync fails the store, as a failed write does" is not true. Acceptable: a failed directory sync removes the renamed file, or the store still writes the.metaand the row.Model: opus-5-5
Decision for finding 1, taking the reviewer's first way: no size comparison on the request path. Compare each cached source's and transformed image's file size with its recorded size in the reconciliation pass the cache runs at startup (
reconcileAccounting), and remove a file that is empty or the wrong size there, with its rows and a warning naming it. A crash or a restored snapshot can only leave a damaged file across a restart, and the new syncs keep fresh writes whole. This removes the database read the PR added to every cache hit, andTestService_Get_WaitsForSlotBeforeReadingCachedSourcepasses unchanged. Keep the plan's other points. For findings 2 and 3 take the reviewer's first way each: write the.metafile like the image (temporary name, synced, renamed), and remove the renamed file when the directory sync fails.Model: opus-5-5
aee645faf8to36c9e1408freconcileVariantFile,reconcileSourceFile);LookupandLookupSourceare as onnext, soTestService_Get_WaitsForSlotBeforeReadingCachedSourceno longer depends on when its request runs, and a hit reads no extra row. This PR's own request-path tests, never onnext, now run the pass before the request..metafile is written like the image (temporary name, synced, renamed, by the samewriteFileSynced) and before it;TestVariantStorage_StoreReplacesMetaFilechecks it is replaced, not written into.Model: opus-5-5
Review passed.
Model: opus-5-5
36c9e1408fto85bf1f64ec85bf1f64ecto36c9e1408f36c9e1408ftoe97673c7f2Rebased onto
next. Files that conflicted:internal/imgcache/service.go:nextnow checks the negative cache after the variant lookup instead of before it (#245). This PR only reworded the comment above that lookup and later put it back, so I keptnext's code, and the PR no longer changes this file.TODO.md: the union merge kept an older second copy of this PR's entry and put newer entries above it. Now there is one copy, at the top of Completed Steps.Model: opus-5-5