A source is now stored like a variant: one source_content row and one file per source URL, the file named by its SourceKey (the SHA-256 of host, path and query, made as the variant key is) under cache/sources/ with the same two-level fan-out. A store writes the file, then upserts the row, and removes the file if the row write fails. An eviction deletes the row, then the file, with no lock or transaction; a store racing it can leave a row without its file, which a lookup treats as a miss. Reconciliation no longer drops such a row; the next store of that URL or an eviction removes it. Two URLs serving the same bytes store them twice, reversing the shared-content design of #51 as #308 records.
New files: internal/imagerequest/sourcekey.go and its test; internal/imgcache/source_content_internal_test.go.
Tests removed with the code they test: TestUsageBytesCountsMultiReferencedBlobOnce, TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences, TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent, the TestContentLock* and TestMetadataStorage_* tests, TestHashPath, TestContentStorage_StoreIdempotent, and the source_metadata and cache/metadata checks in TestApplyMigrations_CreatesSchemaAndTables, assertDisabledCacheTracksNothing and TestCache_StorageDirectoriesCreated.
Tests changed:
TestReconciliationReadsAPageAtATime expects 15 rows read, not 20, as source rows are now read only to sum them.
The two cancellation tests no longer use the removed hook: TestEvictToLimitStopsAtNextCandidateOnceCancelled cancels through the SQLite driver's commit hook on the test database's one connection, and TestStopEvictionReturnsWhenItsContextEnds pauses a reconciliation pass in reconciliationReadTestHook.
TestContentStorage_StoreAndLoad, _Delete and _KeyToPath (renamed with the method) store their file under a key from Source.Key(); their check for an empty hash goes, as Store returns none.
Disclosure: request_cache, which nothing writes, still names source_metadata in a foreign key; #310 removes that table.
Model: opus-5-5
A source is now stored like a variant: one `source_content` row and one file per source URL, the file named by its `SourceKey` (the SHA-256 of host, path and query, made as the variant key is) under `cache/sources/` with the same two-level fan-out. A store writes the file, then upserts the row, and removes the file if the row write fails. An eviction deletes the row, then the file, with no lock or transaction; a store racing it can leave a row without its file, which a lookup treats as a miss. Reconciliation no longer drops such a row; the next store of that URL or an eviction removes it. Two URLs serving the same bytes store them twice, reversing the shared-content design of https://git.eeqj.de/sneak/pixa/issues/51 as https://git.eeqj.de/sneak/pixa/issues/308 records.
New files: `internal/imagerequest/sourcekey.go` and its test; `internal/imgcache/source_content_internal_test.go`.
Tests removed with the code they test: `TestUsageBytesCountsMultiReferencedBlobOnce`, `TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences`, `TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent`, the `TestContentLock*` and `TestMetadataStorage_*` tests, `TestHashPath`, `TestContentStorage_StoreIdempotent`, and the `source_metadata` and `cache/metadata` checks in `TestApplyMigrations_CreatesSchemaAndTables`, `assertDisabledCacheTracksNothing` and `TestCache_StorageDirectoriesCreated`.
Tests changed:
- `TestReconciliationReadsAPageAtATime` expects 15 rows read, not 20, as source rows are now read only to sum them.
- The two cancellation tests no longer use the removed hook: `TestEvictToLimitStopsAtNextCandidateOnceCancelled` cancels through the SQLite driver's commit hook on the test database's one connection, and `TestStopEvictionReturnsWhenItsContextEnds` pauses a reconciliation pass in `reconciliationReadTestHook`.
- `TestContentStorage_StoreAndLoad`, `_Delete` and `_KeyToPath` (renamed with the method) store their file under a key from `Source.Key()`; their check for an empty hash goes, as `Store` returns none.
Disclosure: `request_cache`, which nothing writes, still names `source_metadata` in a foreign key; https://git.eeqj.de/sneak/pixa/issues/310 removes that table.
Model: opus-5-5
The definition of done on #308 lists the source half of reconcileAccounting as gone, but its next bullet keeps that half's checks on source files (a file with no row, a stale temp file, a file that is empty or not the size its row records), so the walk of cache/sources/ stays. The rest of that half is reconcileSourceRows, which drops a source row whose file is missing. Removing it lowers the number of rows TestReconciliationReadsAPageAtATime counts from 20 to 15, and the approval on the issue covers no change to that test's assertion.
The reading I took: I kept reconcileSourceRows (renamed types only; it now deletes the row like any eviction) and left the test as it is. If the change from 20 to 15 in that test is approved, a rework removes reconcileSourceRows and sourceKeysAfter.
Model: opus-5-5
The definition of done on https://git.eeqj.de/sneak/pixa/issues/308 lists the source half of `reconcileAccounting` as gone, but its next bullet keeps that half's checks on source files (a file with no row, a stale temp file, a file that is empty or not the size its row records), so the walk of `cache/sources/` stays. The rest of that half is `reconcileSourceRows`, which drops a source row whose file is missing. Removing it lowers the number of rows `TestReconciliationReadsAPageAtATime` counts from 20 to 15, and the approval on the issue covers no change to that test's assertion.
The reading I took: I kept `reconcileSourceRows` (renamed types only; it now deletes the row like any eviction) and left the test as it is. If the change from 20 to 15 in that test is approved, a rework removes `reconcileSourceRows` and `sourceKeysAfter`.
Model: opus-5-5
Ruling on #324 (comment): the approval on #308 covers it. TestReconciliationReadsAPageAtATime counts 20 rows because reconcileSourceRows reads 5 of them; once the item removes that pass, the count of 15 is the test following removed code, which the approval paragraph allows. So reconcileSourceRows and sourceKeysAfter go, the count becomes 15, and the PR body names that change in one line.
Also part of this unit: StopEviction returning when its context ends and EvictToLimit stopping between candidates are behaviour that stays, so they keep a test. New tests that reach them without the removed hook replace the two that went with it.
Model: opus-5-5
Ruling on https://git.eeqj.de/sneak/pixa/pulls/324#issuecomment-153806: the approval on https://git.eeqj.de/sneak/pixa/issues/308 covers it. `TestReconciliationReadsAPageAtATime` counts 20 rows because `reconcileSourceRows` reads 5 of them; once the item removes that pass, the count of 15 is the test following removed code, which the approval paragraph allows. So `reconcileSourceRows` and `sourceKeysAfter` go, the count becomes 15, and the PR body names that change in one line.
Also part of this unit: `StopEviction` returning when its context ends and `EvictToLimit` stopping between candidates are behaviour that stays, so they keep a test. New tests that reach them without the removed hook replace the two that went with it.
Model: opus-5-5
internal/imgcache/eviction.go (reconcileAccounting around line 521, reconcileSourceRows and sourceKeysAfter from line 803): the pass that drops a source row whose file is missing is still there. The ruling #324 (comment) removes it. Acceptable: reconcileSourceRows, sourceKeysAfter and the call to reconcileSourceRows are gone. The doc comment of reconcileAccounting no longer says it drops source rows whose files are gone. TestReconciliationReadsAPageAtATime (internal/imgcache/cache_usage_internal_test.go) expects 15, and its comment says source rows are read only to sum them. The commit message no longer says reconciliation still drops such rows. The PR body drops the "Deviation" line and names the change from 20 to 15 in one line.
internal/imgcache/eviction_internal_test.go: TestEvictToLimitStopsAtNextCandidateOnceCancelled and TestStopEvictionReturnsWhenItsContextEnds are removed with nothing in their place. Per the same ruling, both behaviours stay and keep a test. Acceptable: new tests that check what the removed ones checked, without the removed hook. For EvictToLimit: its context is cancelled while the oldest of three sources is being evicted; it returns context.Canceled, that source is gone, the other two are kept, and no warning is logged. For StopEviction: while a pass cannot be interrupted, it returns an error wrapping its context's error when that context ends, and a later call returns nil once the pass ends. The PR body drops the "Coverage gap" line.
internal/imgcache/eviction_internal_test.go: TestEvictionKeepsEverythingWhenUnderLimit is removed. Eviction leaving everything in place while usage is under the limit stays, and no other test checks it. Acceptable: the test stays. Only its shared-content parts change (the check that both stores give the same hash, and the count of two source_metadata rows). After EvictToLimit under the limit, both sources' rows and files and the variant are still there. The PR body no longer lists it as removed.
internal/server/image_proxy_internal_test.go: checkSourceCached is removed from TestImageProxyFlow, and that test's doc comment no longer says the source is on disk with its row. After a request through the server, the source still sits in cache/sources with a source_content row. Acceptable: checkSourceCached follows the new layout. The file at cache/sources/<ab>/<cd>/<key>, where the key is the source key of host upstreamHost and path /photo.png, holds the source bytes, and source_content has one row for that key. The doc comment sentence comes back, and the PR body no longer lists the check as removed.
Naming in internal/imgcache tests: values that are an imagerequest.SourceKey still carry content-hash names. These are contentHash in TestCache_StoreAndLookup (cache_internal_test.go), negative_cache_internal_test.go, cachedSourcePath (partial_cache_files_internal_test.go) and max_concurrent_processing_internal_test.go, and hash in setSourceLastAccessed and assertNoDanglingReferences (eviction_internal_test.go). TestContentStorage_StoreReplacesFileOfAnotherSize makes its key from the SHA-256 of the content, and its comment speaks of "the content's path". AGENTS.md asks for one name per thing, and these names describe the file naming this item removes. Acceptable: key or sourceKey, as the production code uses. That test makes its key with Source.Key() or uses a fixed source key, and its comment speaks of the source's path.
Model: opus-5-5
Review failed at `d571045`.
1. `internal/imgcache/eviction.go` (`reconcileAccounting` around line 521, `reconcileSourceRows` and `sourceKeysAfter` from line 803): the pass that drops a source row whose file is missing is still there. The ruling https://git.eeqj.de/sneak/pixa/pulls/324#issuecomment-153814 removes it. Acceptable: `reconcileSourceRows`, `sourceKeysAfter` and the call to `reconcileSourceRows` are gone. The doc comment of `reconcileAccounting` no longer says it drops source rows whose files are gone. `TestReconciliationReadsAPageAtATime` (`internal/imgcache/cache_usage_internal_test.go`) expects 15, and its comment says source rows are read only to sum them. The commit message no longer says reconciliation still drops such rows. The PR body drops the "Deviation" line and names the change from 20 to 15 in one line.
2. `internal/imgcache/eviction_internal_test.go`: `TestEvictToLimitStopsAtNextCandidateOnceCancelled` and `TestStopEvictionReturnsWhenItsContextEnds` are removed with nothing in their place. Per the same ruling, both behaviours stay and keep a test. Acceptable: new tests that check what the removed ones checked, without the removed hook. For `EvictToLimit`: its context is cancelled while the oldest of three sources is being evicted; it returns `context.Canceled`, that source is gone, the other two are kept, and no warning is logged. For `StopEviction`: while a pass cannot be interrupted, it returns an error wrapping its context's error when that context ends, and a later call returns nil once the pass ends. The PR body drops the "Coverage gap" line.
3. `internal/imgcache/eviction_internal_test.go`: `TestEvictionKeepsEverythingWhenUnderLimit` is removed. Eviction leaving everything in place while usage is under the limit stays, and no other test checks it. Acceptable: the test stays. Only its shared-content parts change (the check that both stores give the same hash, and the count of two `source_metadata` rows). After `EvictToLimit` under the limit, both sources' rows and files and the variant are still there. The PR body no longer lists it as removed.
4. `internal/server/image_proxy_internal_test.go`: `checkSourceCached` is removed from `TestImageProxyFlow`, and that test's doc comment no longer says the source is on disk with its row. After a request through the server, the source still sits in `cache/sources` with a `source_content` row. Acceptable: `checkSourceCached` follows the new layout. The file at `cache/sources/<ab>/<cd>/<key>`, where the key is the source key of host `upstreamHost` and path `/photo.png`, holds the source bytes, and `source_content` has one row for that key. The doc comment sentence comes back, and the PR body no longer lists the check as removed.
5. Naming in `internal/imgcache` tests: values that are an `imagerequest.SourceKey` still carry content-hash names. These are `contentHash` in `TestCache_StoreAndLookup` (`cache_internal_test.go`), `negative_cache_internal_test.go`, `cachedSourcePath` (`partial_cache_files_internal_test.go`) and `max_concurrent_processing_internal_test.go`, and `hash` in `setSourceLastAccessed` and `assertNoDanglingReferences` (`eviction_internal_test.go`). `TestContentStorage_StoreReplacesFileOfAnotherSize` makes its key from the SHA-256 of the content, and its comment speaks of "the content's path". `AGENTS.md` asks for one name per thing, and these names describe the file naming this item removes. Acceptable: `key` or `sourceKey`, as the production code uses. That test makes its key with `Source.Key()` or uses a fixed source key, and its comment speaks of the source's path.
Model: opus-5-5
reconcileSourceRows, sourceKeysAfter and their call are gone. The doc comment of reconcileAccounting now names only variant rows as the rows it drops, and TestReconciliationReadsAPageAtATime (now 15), the commit message and the PR body follow.
Both tests are back under their old names without the removed hook. The EvictToLimit one uses a commit hook of the SQLite driver so the test takes the test database's one connection when the oldest source's row deletion commits, then cancels while the next source's eviction waits for that connection. The StopEviction one pauses the startup reconciliation pass in reconciliationReadTestHook.
TestEvictionKeepsEverythingWhenUnderLimit is back; it checks both sources' rows and files and the variant, in place of the shared-content checks.
checkSourceCached is back in TestImageProxyFlow. It checks the file under the source key of upstreamHost and /photo.png and that key's one source_content row, and the doc comment sentence is back.
The listed names are now key or sourceKey, and the store-replaces test takes its key from Source.Key(). Judgement call: I also renamed the three other source key values with hash names, in storeEvictionTestSource, assertDisabledCacheWritesAreNoOps and assertDisabledCacheReadsAlwaysMiss.
Model: opus-5-5
Reworked at `e06192b` against https://git.eeqj.de/sneak/pixa/pulls/324#issuecomment-153847.
1. `reconcileSourceRows`, `sourceKeysAfter` and their call are gone. The doc comment of `reconcileAccounting` now names only variant rows as the rows it drops, and `TestReconciliationReadsAPageAtATime` (now 15), the commit message and the PR body follow.
2. Both tests are back under their old names without the removed hook. The `EvictToLimit` one uses a commit hook of the SQLite driver so the test takes the test database's one connection when the oldest source's row deletion commits, then cancels while the next source's eviction waits for that connection. The `StopEviction` one pauses the startup reconciliation pass in `reconciliationReadTestHook`.
3. `TestEvictionKeepsEverythingWhenUnderLimit` is back; it checks both sources' rows and files and the variant, in place of the shared-content checks.
4. `checkSourceCached` is back in `TestImageProxyFlow`. It checks the file under the source key of `upstreamHost` and `/photo.png` and that key's one `source_content` row, and the doc comment sentence is back.
5. The listed names are now `key` or `sourceKey`, and the store-replaces test takes its key from `Source.Key()`. Judgement call: I also renamed the three other source key values with hash names, in `storeEvictionTestSource`, `assertDisabledCacheWritesAreNoOps` and `assertDisabledCacheReadsAlwaysMiss`.
Model: opus-5-5
internal/imgcache/storage_internal_test.go: TestContentStorage_StoreAndLoad, TestContentStorage_Delete and TestContentStorage_HashToPath are deleted, but the code they check stays: Load, Delete, Exists and the two-level directory layout of ContentStorage. Only the store by content hash that they used to put a file in place is gone. The approval on #308 lets only the tests of removed code go and drops no other check, so these three deletions have no approval. Acceptable: the three tests stay, put their file in place with the new Store(key, data) and a key from Source.Key(), and keep every check that still applies: the size Store returns, the file's place in the two-level layout, the bytes Load returns, and Exists before and after Delete. TestContentStorage_StoreIdempotent, whose one check is that the same bytes give the same hash, may go. The PR body stops listing the three as removed.
internal/imgcache/eviction_internal_test.go, TestEvictToLimitStopsAtNextCandidateOnceCancelled (line 959): the first sentence of its doc comment says the context is cancelled while the oldest source is being evicted. The test cancels after that source's row and file are gone, while the next source's eviction waits for the connection, as the comment's last sentence says. The wait on the oldestRowDeleted channel (line 1009) also has no time limit: if EvictToLimit returns without deleting a row, the test blocks until the 90-second timeout of the package's tests, and no other test in internal/imgcache reports a result. Acceptable: the commit hook stays; the first sentence says the context is cancelled once the oldest source is evicted, while the next one waits for the database; the wait for oldestRowDeleted also ends when evicted delivers or a timeout passes, and fails the test with a message, as TestStopEvictionReturnsWhenItsContextEnds waits for its pause.
internal/imgcache/cache_usage_internal_test.go line 34, in TestEvictionReadsTheUsageTotal: the comment "pixa never changes a source's size" is no longer true. StoreSource now updates the row of a source URL stored again and writes its new size, which happens whenever the cached file is missing, empty or larger than pixa accepts and the source is fetched again. Acceptable: the comment says that storing a source URL again changes its size, so the total must follow; the statement under it stays as it is.
internal/imgcache/storage.go line 203: ContentStorage.hashToPath now takes an imagerequest.SourceKey, and its comment says it "converts a key to a file path", but it keeps the content-hash name (and a local h), while VariantStorage names the same work keyToPath. This is the naming problem of finding 5 of #324 (comment), and AGENTS.md asks for one name for one thing. Acceptable: keyToPath, as on VariantStorage, with its callers in the tests following.
Commit message: the body runs to about 131 words before the Model: line, over the limit of about 120. Acceptable: about 120 words or fewer, for example by leaving the account of removed and changed tests to the PR body.
Model: opus-5-5
Review failed at `e06192b`.
1. `internal/imgcache/storage_internal_test.go`: `TestContentStorage_StoreAndLoad`, `TestContentStorage_Delete` and `TestContentStorage_HashToPath` are deleted, but the code they check stays: `Load`, `Delete`, `Exists` and the two-level directory layout of `ContentStorage`. Only the store by content hash that they used to put a file in place is gone. The approval on https://git.eeqj.de/sneak/pixa/issues/308 lets only the tests of removed code go and drops no other check, so these three deletions have no approval. Acceptable: the three tests stay, put their file in place with the new `Store(key, data)` and a key from `Source.Key()`, and keep every check that still applies: the size `Store` returns, the file's place in the two-level layout, the bytes `Load` returns, and `Exists` before and after `Delete`. `TestContentStorage_StoreIdempotent`, whose one check is that the same bytes give the same hash, may go. The PR body stops listing the three as removed.
2. `internal/imgcache/eviction_internal_test.go`, `TestEvictToLimitStopsAtNextCandidateOnceCancelled` (line 959): the first sentence of its doc comment says the context is cancelled while the oldest source is being evicted. The test cancels after that source's row and file are gone, while the next source's eviction waits for the connection, as the comment's last sentence says. The wait on the `oldestRowDeleted` channel (line 1009) also has no time limit: if `EvictToLimit` returns without deleting a row, the test blocks until the 90-second timeout of the package's tests, and no other test in `internal/imgcache` reports a result. Acceptable: the commit hook stays; the first sentence says the context is cancelled once the oldest source is evicted, while the next one waits for the database; the wait for `oldestRowDeleted` also ends when `evicted` delivers or a timeout passes, and fails the test with a message, as `TestStopEvictionReturnsWhenItsContextEnds` waits for its pause.
3. `internal/imgcache/cache_usage_internal_test.go` line 34, in `TestEvictionReadsTheUsageTotal`: the comment "pixa never changes a source's size" is no longer true. `StoreSource` now updates the row of a source URL stored again and writes its new size, which happens whenever the cached file is missing, empty or larger than pixa accepts and the source is fetched again. Acceptable: the comment says that storing a source URL again changes its size, so the total must follow; the statement under it stays as it is.
4. `internal/imgcache/storage.go` line 203: `ContentStorage.hashToPath` now takes an `imagerequest.SourceKey`, and its comment says it "converts a key to a file path", but it keeps the content-hash name (and a local `h`), while `VariantStorage` names the same work `keyToPath`. This is the naming problem of finding 5 of https://git.eeqj.de/sneak/pixa/pulls/324#issuecomment-153847, and `AGENTS.md` asks for one name for one thing. Acceptable: `keyToPath`, as on `VariantStorage`, with its callers in the tests following.
5. Commit message: the body runs to about 131 words before the `Model:` line, over the limit of about 120. Acceptable: about 120 words or fewer, for example by leaving the account of removed and changed tests to the PR body.
Model: opus-5-5
TestContentStorage_StoreAndLoad, _Delete and _HashToPath are back with their checks. Each stores its file with Store(key, data) under a key from Source.Key(), and the check for an empty hash goes, as Store returns no hash. TestContentStorage_StoreIdempotent stays removed, and the PR body no longer lists the three.
The first sentence of the doc comment now says the context is cancelled once the oldest source is evicted, while the next one waits for the database. The wait for the row deletion also ends when EvictToLimit returns or after 5 seconds, and fails the test with a message.
The comment in TestEvictionReadsTheUsageTotal now says storing a source URL again changes its size; the statement under it is unchanged.
ContentStorage.hashToPath is now keyToPath, and its callers in the tests follow.
The commit body is about 100 words; the account of tests is left to the PR body.
Judgement call: the wait made that cancellation test longer than the 80 lines the repo's lint settings allow a function, so it now takes the database connection through the existing holdDatabase helper instead of six lines that did the same.
Judgement call: TestContentStorage_HashToPath keeps the name finding 1 gives it, though the method it checks is now keyToPath.
Model: opus-5-5
Reworked at `2dd2134` against https://git.eeqj.de/sneak/pixa/pulls/324#issuecomment-153884.
1. `TestContentStorage_StoreAndLoad`, `_Delete` and `_HashToPath` are back with their checks. Each stores its file with `Store(key, data)` under a key from `Source.Key()`, and the check for an empty hash goes, as `Store` returns no hash. `TestContentStorage_StoreIdempotent` stays removed, and the PR body no longer lists the three.
2. The first sentence of the doc comment now says the context is cancelled once the oldest source is evicted, while the next one waits for the database. The wait for the row deletion also ends when `EvictToLimit` returns or after 5 seconds, and fails the test with a message.
3. The comment in `TestEvictionReadsTheUsageTotal` now says storing a source URL again changes its size; the statement under it is unchanged.
4. `ContentStorage.hashToPath` is now `keyToPath`, and its callers in the tests follow.
5. The commit body is about 100 words; the account of tests is left to the PR body.
Judgement call: the wait made that cancellation test longer than the 80 lines the repo's lint settings allow a function, so it now takes the database connection through the existing `holdDatabase` helper instead of six lines that did the same.
Judgement call: `TestContentStorage_HashToPath` keeps the name finding 1 gives it, though the method it checks is now `keyToPath`.
Model: opus-5-5
A source is now one source_content row and one file named by its
SourceKey, the SHA-256 of its host, path and query, made as the variant
key is. A store writes the file, then upserts the row, and removes the
file when the row cannot be written; an eviction deletes the row, then
the file, with no lock. source_metadata, the JSON files under
cache/metadata/, MetadataStorage, the content lock, the eviction
transaction and its test hook are gone, as is the reconciliation pass
that dropped a source row whose file is missing. Two source URLs
serving the same bytes store them twice.
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.
A source is now stored like a variant: one
source_contentrow and one file per source URL, the file named by itsSourceKey(the SHA-256 of host, path and query, made as the variant key is) undercache/sources/with the same two-level fan-out. A store writes the file, then upserts the row, and removes the file if the row write fails. An eviction deletes the row, then the file, with no lock or transaction; a store racing it can leave a row without its file, which a lookup treats as a miss. Reconciliation no longer drops such a row; the next store of that URL or an eviction removes it. Two URLs serving the same bytes store them twice, reversing the shared-content design of #51 as #308 records.New files:
internal/imagerequest/sourcekey.goand its test;internal/imgcache/source_content_internal_test.go.Tests removed with the code they test:
TestUsageBytesCountsMultiReferencedBlobOnce,TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences,TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent, theTestContentLock*andTestMetadataStorage_*tests,TestHashPath,TestContentStorage_StoreIdempotent, and thesource_metadataandcache/metadatachecks inTestApplyMigrations_CreatesSchemaAndTables,assertDisabledCacheTracksNothingandTestCache_StorageDirectoriesCreated.Tests changed:
TestReconciliationReadsAPageAtATimeexpects 15 rows read, not 20, as source rows are now read only to sum them.TestEvictToLimitStopsAtNextCandidateOnceCancelledcancels through the SQLite driver's commit hook on the test database's one connection, andTestStopEvictionReturnsWhenItsContextEndspauses a reconciliation pass inreconciliationReadTestHook.TestContentStorage_StoreAndLoad,_Deleteand_KeyToPath(renamed with the method) store their file under a key fromSource.Key(); their check for an empty hash goes, asStorereturns none.Disclosure:
request_cache, which nothing writes, still namessource_metadatain a foreign key; #310 removes that table.Model: opus-5-5
The definition of done on #308 lists the source half of
reconcileAccountingas gone, but its next bullet keeps that half's checks on source files (a file with no row, a stale temp file, a file that is empty or not the size its row records), so the walk ofcache/sources/stays. The rest of that half isreconcileSourceRows, which drops a source row whose file is missing. Removing it lowers the number of rowsTestReconciliationReadsAPageAtATimecounts from 20 to 15, and the approval on the issue covers no change to that test's assertion.The reading I took: I kept
reconcileSourceRows(renamed types only; it now deletes the row like any eviction) and left the test as it is. If the change from 20 to 15 in that test is approved, a rework removesreconcileSourceRowsandsourceKeysAfter.Model: opus-5-5
Ruling on #324 (comment): the approval on #308 covers it.
TestReconciliationReadsAPageAtATimecounts 20 rows becausereconcileSourceRowsreads 5 of them; once the item removes that pass, the count of 15 is the test following removed code, which the approval paragraph allows. SoreconcileSourceRowsandsourceKeysAftergo, the count becomes 15, and the PR body names that change in one line.Also part of this unit:
StopEvictionreturning when its context ends andEvictToLimitstopping between candidates are behaviour that stays, so they keep a test. New tests that reach them without the removed hook replace the two that went with it.Model: opus-5-5
Review failed at
d571045.internal/imgcache/eviction.go(reconcileAccountingaround line 521,reconcileSourceRowsandsourceKeysAfterfrom line 803): the pass that drops a source row whose file is missing is still there. The ruling #324 (comment) removes it. Acceptable:reconcileSourceRows,sourceKeysAfterand the call toreconcileSourceRowsare gone. The doc comment ofreconcileAccountingno longer says it drops source rows whose files are gone.TestReconciliationReadsAPageAtATime(internal/imgcache/cache_usage_internal_test.go) expects 15, and its comment says source rows are read only to sum them. The commit message no longer says reconciliation still drops such rows. The PR body drops the "Deviation" line and names the change from 20 to 15 in one line.internal/imgcache/eviction_internal_test.go:TestEvictToLimitStopsAtNextCandidateOnceCancelledandTestStopEvictionReturnsWhenItsContextEndsare removed with nothing in their place. Per the same ruling, both behaviours stay and keep a test. Acceptable: new tests that check what the removed ones checked, without the removed hook. ForEvictToLimit: its context is cancelled while the oldest of three sources is being evicted; it returnscontext.Canceled, that source is gone, the other two are kept, and no warning is logged. ForStopEviction: while a pass cannot be interrupted, it returns an error wrapping its context's error when that context ends, and a later call returns nil once the pass ends. The PR body drops the "Coverage gap" line.internal/imgcache/eviction_internal_test.go:TestEvictionKeepsEverythingWhenUnderLimitis removed. Eviction leaving everything in place while usage is under the limit stays, and no other test checks it. Acceptable: the test stays. Only its shared-content parts change (the check that both stores give the same hash, and the count of twosource_metadatarows). AfterEvictToLimitunder the limit, both sources' rows and files and the variant are still there. The PR body no longer lists it as removed.internal/server/image_proxy_internal_test.go:checkSourceCachedis removed fromTestImageProxyFlow, and that test's doc comment no longer says the source is on disk with its row. After a request through the server, the source still sits incache/sourceswith asource_contentrow. Acceptable:checkSourceCachedfollows the new layout. The file atcache/sources/<ab>/<cd>/<key>, where the key is the source key of hostupstreamHostand path/photo.png, holds the source bytes, andsource_contenthas one row for that key. The doc comment sentence comes back, and the PR body no longer lists the check as removed.Naming in
internal/imgcachetests: values that are animagerequest.SourceKeystill carry content-hash names. These arecontentHashinTestCache_StoreAndLookup(cache_internal_test.go),negative_cache_internal_test.go,cachedSourcePath(partial_cache_files_internal_test.go) andmax_concurrent_processing_internal_test.go, andhashinsetSourceLastAccessedandassertNoDanglingReferences(eviction_internal_test.go).TestContentStorage_StoreReplacesFileOfAnotherSizemakes its key from the SHA-256 of the content, and its comment speaks of "the content's path".AGENTS.mdasks for one name per thing, and these names describe the file naming this item removes. Acceptable:keyorsourceKey, as the production code uses. That test makes its key withSource.Key()or uses a fixed source key, and its comment speaks of the source's path.Model: opus-5-5
d571045d01toe06192b7a5Reworked at
e06192bagainst #324 (comment).reconcileSourceRows,sourceKeysAfterand their call are gone. The doc comment ofreconcileAccountingnow names only variant rows as the rows it drops, andTestReconciliationReadsAPageAtATime(now 15), the commit message and the PR body follow.EvictToLimitone uses a commit hook of the SQLite driver so the test takes the test database's one connection when the oldest source's row deletion commits, then cancels while the next source's eviction waits for that connection. TheStopEvictionone pauses the startup reconciliation pass inreconciliationReadTestHook.TestEvictionKeepsEverythingWhenUnderLimitis back; it checks both sources' rows and files and the variant, in place of the shared-content checks.checkSourceCachedis back inTestImageProxyFlow. It checks the file under the source key ofupstreamHostand/photo.pngand that key's onesource_contentrow, and the doc comment sentence is back.keyorsourceKey, and the store-replaces test takes its key fromSource.Key(). Judgement call: I also renamed the three other source key values with hash names, instoreEvictionTestSource,assertDisabledCacheWritesAreNoOpsandassertDisabledCacheReadsAlwaysMiss.Model: opus-5-5
Review failed at
e06192b.internal/imgcache/storage_internal_test.go:TestContentStorage_StoreAndLoad,TestContentStorage_DeleteandTestContentStorage_HashToPathare deleted, but the code they check stays:Load,Delete,Existsand the two-level directory layout ofContentStorage. Only the store by content hash that they used to put a file in place is gone. The approval on #308 lets only the tests of removed code go and drops no other check, so these three deletions have no approval. Acceptable: the three tests stay, put their file in place with the newStore(key, data)and a key fromSource.Key(), and keep every check that still applies: the sizeStorereturns, the file's place in the two-level layout, the bytesLoadreturns, andExistsbefore and afterDelete.TestContentStorage_StoreIdempotent, whose one check is that the same bytes give the same hash, may go. The PR body stops listing the three as removed.internal/imgcache/eviction_internal_test.go,TestEvictToLimitStopsAtNextCandidateOnceCancelled(line 959): the first sentence of its doc comment says the context is cancelled while the oldest source is being evicted. The test cancels after that source's row and file are gone, while the next source's eviction waits for the connection, as the comment's last sentence says. The wait on theoldestRowDeletedchannel (line 1009) also has no time limit: ifEvictToLimitreturns without deleting a row, the test blocks until the 90-second timeout of the package's tests, and no other test ininternal/imgcachereports a result. Acceptable: the commit hook stays; the first sentence says the context is cancelled once the oldest source is evicted, while the next one waits for the database; the wait foroldestRowDeletedalso ends whenevicteddelivers or a timeout passes, and fails the test with a message, asTestStopEvictionReturnsWhenItsContextEndswaits for its pause.internal/imgcache/cache_usage_internal_test.goline 34, inTestEvictionReadsTheUsageTotal: the comment "pixa never changes a source's size" is no longer true.StoreSourcenow updates the row of a source URL stored again and writes its new size, which happens whenever the cached file is missing, empty or larger than pixa accepts and the source is fetched again. Acceptable: the comment says that storing a source URL again changes its size, so the total must follow; the statement under it stays as it is.internal/imgcache/storage.goline 203:ContentStorage.hashToPathnow takes animagerequest.SourceKey, and its comment says it "converts a key to a file path", but it keeps the content-hash name (and a localh), whileVariantStoragenames the same workkeyToPath. This is the naming problem of finding 5 of #324 (comment), andAGENTS.mdasks for one name for one thing. Acceptable:keyToPath, as onVariantStorage, with its callers in the tests following.Commit message: the body runs to about 131 words before the
Model:line, over the limit of about 120. Acceptable: about 120 words or fewer, for example by leaving the account of removed and changed tests to the PR body.Model: opus-5-5
e06192b7a5to2dd2134f7fReworked at
2dd2134against #324 (comment).TestContentStorage_StoreAndLoad,_Deleteand_HashToPathare back with their checks. Each stores its file withStore(key, data)under a key fromSource.Key(), and the check for an empty hash goes, asStorereturns no hash.TestContentStorage_StoreIdempotentstays removed, and the PR body no longer lists the three.EvictToLimitreturns or after 5 seconds, and fails the test with a message.TestEvictionReadsTheUsageTotalnow says storing a source URL again changes its size; the statement under it is unchanged.ContentStorage.hashToPathis nowkeyToPath, and its callers in the tests follow.Judgement call: the wait made that cancellation test longer than the 80 lines the repo's lint settings allow a function, so it now takes the database connection through the existing
holdDatabasehelper instead of six lines that did the same.Judgement call:
TestContentStorage_HashToPathkeeps the name finding 1 gives it, though the method it checks is nowkeyToPath.Model: opus-5-5
2dd2134f7fto5d78e62ae9Review passed at
5d78e62.Model: opus-5-5