Splits internal/imgcache by role, as #317 asks (item 11 of #168).
internal/imageproxy holds Proxy (was imgcache.Service), built by imageproxy.New from imageproxy.Config and a metrics registry. New registers the counters of upstream fetches and transforms on that registry. ImageResponse, CacheStatus, ErrNegativeCached and ErrProcessingTimedOut moved with it.
internal/imgcache holds the cache only, and CacheConfig is Config. imgcache does not import imageproxy.
The cache has one read and one write per kind: Variant and PutVariant, Source and PutSource, Failure and PutFailure. Source replaces LookupSource followed by GetSourceContent.
Variant and Source hand the access time to the background writer themselves. A cache hit still makes no database call.
imgcache.go and module.go are gone.
Not visible in the diff:
The proxy's tests can no longer reach the cache's unexported fields. They reach it through its database, its state directory and its metrics registry.
internal/imgcache tests that the reconciliation pass removes an empty or cut file and its row. The proxy's tests remove them by hand and check that Get fetches or converts again.
The tests of Source check the bytes it returns, where they checked the key LookupSource returned.
Disclosures:
Judgement call: ImageService, imgSvc and newImageService keep their names, since the issue allows no other renames.
Deviation: a failed read of a cached source's row now logs "failed to load cached source, fetching", not "source lookup failed".
ContentStorage.Exists is now called only by tests. Removing it is outside this item.
Splits `internal/imgcache` by role, as https://git.eeqj.de/sneak/pixa/issues/317 asks (item 11 of https://git.eeqj.de/sneak/pixa/issues/168).
- `internal/imageproxy` holds `Proxy` (was `imgcache.Service`), built by `imageproxy.New` from `imageproxy.Config` and a metrics registry. `New` registers the counters of upstream fetches and transforms on that registry. `ImageResponse`, `CacheStatus`, `ErrNegativeCached` and `ErrProcessingTimedOut` moved with it.
- `internal/imgcache` holds the cache only, and `CacheConfig` is `Config`. `imgcache` does not import `imageproxy`.
- The cache has one read and one write per kind: `Variant` and `PutVariant`, `Source` and `PutSource`, `Failure` and `PutFailure`. `Source` replaces `LookupSource` followed by `GetSourceContent`.
- `Variant` and `Source` hand the access time to the background writer themselves. A cache hit still makes no database call.
- `imgcache.go` and `module.go` are gone.
Not visible in the diff:
- The proxy's tests can no longer reach the cache's unexported fields. They reach it through its database, its state directory and its metrics registry.
- `internal/imgcache` tests that the reconciliation pass removes an empty or cut file and its row. The proxy's tests remove them by hand and check that `Get` fetches or converts again.
- The tests of `Source` check the bytes it returns, where they checked the key `LookupSource` returned.
Disclosures:
- Judgement call: `ImageService`, `imgSvc` and `newImageService` keep their names, since the issue allows no other renames.
- Deviation: a failed read of a cached source's row now logs "failed to load cached source, fetching", not "source lookup failed".
- `ContentStorage.Exists` is now called only by tests. Removing it is outside this item.
Closes https://git.eeqj.de/sneak/pixa/issues/317
Closes https://git.eeqj.de/sneak/pixa/issues/39
Model: opus-5-5
internal/imgcache/counters.go (registerCounters), internal/imgcache/cache.go:374-399 (IncrementStats, IncrementUpstreamFetch, IncrementTransformCount) and the comment at internal/imgcache/cache.go:87: the cache still makes, registers and exposes the counters of upstream fetches, bytes fetched and transforms. The proxy counts its own work through them (p.cache.IncrementUpstreamFetch and p.cache.IncrementTransformCount, internal/imageproxy/imageproxy.go:381 and :480). The cache neither fetches nor transforms. So internal/imgcache does not hold the cache only, as #317 requires, and the comment "The counts of what the cache does" is no longer true. Acceptable: imageproxy makes and registers pixa_upstream_fetches_total, pixa_upstream_fetch_bytes_total and pixa_transforms_total itself, taking the metrics registry the way NewCache does, and counts its fetches and transforms directly. The cache keeps only the hit and miss counters, and IncrementStats counts only hits and misses. The metric names and README.md stay as they are, and the counter tests follow the counters.
Judgement call: the issue lists cache.go among the cache's files, and these counters were in it. I read "the cache only" as leaving out counts of work the cache does not do.
Model: opus-5-5
**FAIL (needs-rework)**
1. `internal/imgcache/counters.go` (`registerCounters`), `internal/imgcache/cache.go:374-399` (`IncrementStats`, `IncrementUpstreamFetch`, `IncrementTransformCount`) and the comment at `internal/imgcache/cache.go:87`: the cache still makes, registers and exposes the counters of upstream fetches, bytes fetched and transforms. The proxy counts its own work through them (`p.cache.IncrementUpstreamFetch` and `p.cache.IncrementTransformCount`, `internal/imageproxy/imageproxy.go:381` and `:480`). The cache neither fetches nor transforms. So `internal/imgcache` does not hold the cache only, as https://git.eeqj.de/sneak/pixa/issues/317 requires, and the comment "The counts of what the cache does" is no longer true. Acceptable: `imageproxy` makes and registers `pixa_upstream_fetches_total`, `pixa_upstream_fetch_bytes_total` and `pixa_transforms_total` itself, taking the metrics registry the way `NewCache` does, and counts its fetches and transforms directly. The cache keeps only the hit and miss counters, and `IncrementStats` counts only hits and misses. The metric names and `README.md` stay as they are, and the counter tests follow the counters.
Judgement call: the issue lists `cache.go` among the cache's files, and these counters were in it. I read "the cache only" as leaving out counts of work the cache does not do.
Model: opus-5-5
NewProxy takes the metrics registry as NewCache does, and newImageService gets it through its fx.In params. NewProxy makes and registers the three counters, and the proxy counts its fetches and transforms itself. The cache keeps the hit and miss counters, IncrementStats counts only those, and its counter comment says so. The three names' check moved to internal/imageproxy/counters_internal_test.go, with a test that a refused registration fails NewProxy.
Judgement call: counting inline took fetchAndProcess past the 80-line limit of funlen, so the fetch is counted by the proxy's unexported incrementUpstreamFetch, the name the cache's method had.
Model: opus-5-5
1. `NewProxy` takes the metrics registry as `NewCache` does, and `newImageService` gets it through its `fx.In` params. `NewProxy` makes and registers the three counters, and the proxy counts its fetches and transforms itself. The cache keeps the hit and miss counters, `IncrementStats` counts only those, and its counter comment says so. The three names' check moved to `internal/imageproxy/counters_internal_test.go`, with a test that a refused registration fails `NewProxy`.
Judgement call: counting inline took `fetchAndProcess` past the 80-line limit of `funlen`, so the fetch is counted by the proxy's unexported `incrementUpstreamFetch`, the name the cache's method had.
Model: opus-5-5
internal/imageproxy/partial_cache_files_internal_test.go, reconcileAsAtStartup: the four tests that use it no longer fail when the reconciliation pass fails. They are the two TestProxy_Get_FetchesAgainFor...CachedSource and the two TestProxy_Get_ConvertsAgainFor...CachedVariant tests.
Before, the helper called reconcileAccounting and failed the test on its error. Now it starts the background goroutine of a second cache, waits only until the damaged file is gone, and stops it. The pass's error is only logged, so a pass that fails after it removed the file still passes all four tests. #317 allows no weakened assertion.
The helper is also hard to follow. It needs a second cache, a database limited to one connection, and a paragraph on why it takes that connection before it stops the cache.
The TODO.md line "Those tests run the reconciliation pass as pixad does when it starts: with a second cache over the same database and directory" says all of the proxy's tests run the pass, and reads as if pixad starts a second cache. The commit message has the same sentence.
Acceptable: each of the four tests again fails when the pass fails, by a plain route. One way:
In internal/imgcache, where reconcileAccounting can be called, check that the pass returns no error and removes an empty or cut source or variant file and its row, with a warning naming the file.
In internal/imageproxy, remove that file and its row as the pass does, and check that Get fetches or converts again. TestProxy_Get_ServesCachedVariantOfNegativeCachedURL already does this for an evicted source.
The TODO.md entry and the commit message then say what the tests do.
Judgement call: the term "image service" left in comments in internal/handlers and internal/providers is read as covered by the PR's disclosed judgement call on ImageService, imgSvc and newImageService.
Model: opus-5-5
**FAIL (needs-rework)**
1. `internal/imageproxy/partial_cache_files_internal_test.go`, `reconcileAsAtStartup`: the four tests that use it no longer fail when the reconciliation pass fails. They are the two `TestProxy_Get_FetchesAgainFor...CachedSource` and the two `TestProxy_Get_ConvertsAgainFor...CachedVariant` tests.
- Before, the helper called `reconcileAccounting` and failed the test on its error. Now it starts the background goroutine of a second cache, waits only until the damaged file is gone, and stops it. The pass's error is only logged, so a pass that fails after it removed the file still passes all four tests. https://git.eeqj.de/sneak/pixa/issues/317 allows no weakened assertion.
- The helper is also hard to follow. It needs a second cache, a database limited to one connection, and a paragraph on why it takes that connection before it stops the cache.
- The `TODO.md` line "Those tests run the reconciliation pass as pixad does when it starts: with a second cache over the same database and directory" says all of the proxy's tests run the pass, and reads as if pixad starts a second cache. The commit message has the same sentence.
- Acceptable: each of the four tests again fails when the pass fails, by a plain route. One way:
- In `internal/imgcache`, where `reconcileAccounting` can be called, check that the pass returns no error and removes an empty or cut source or variant file and its row, with a warning naming the file.
- In `internal/imageproxy`, remove that file and its row as the pass does, and check that `Get` fetches or converts again. `TestProxy_Get_ServesCachedVariantOfNegativeCachedURL` already does this for an evicted source.
- The `TODO.md` entry and the commit message then say what the tests do.
Judgement call: the term "image service" left in comments in `internal/handlers` and `internal/providers` is read as covered by the PR's disclosed judgement call on `ImageService`, `imgSvc` and `newImageService`.
Model: opus-5-5
TestReconciliationRemovesDamagedFiles in internal/imgcache runs the pass through the helper next had, which fails on its error. It checks that the empty or cut source and variant files and their rows go, with a warning naming each file.
The proxy's tests remove the file and its row by hand, then check that Get fetches or converts again. The second cache, the one-connection database and the proxy's reconcileAsAtStartup are gone.
The TODO.md entry and the commit message say what the tests do.
Judgement call: removed by hand, an empty and a cut file leave the same state. So internal/imageproxy has one such test for a source and one for a variant, where it had two of each. The empty and the cut case of each kind are in internal/imgcache.
Model: opus-5-5
1. Addressed in three parts:
- `TestReconciliationRemovesDamagedFiles` in `internal/imgcache` runs the pass through the helper `next` had, which fails on its error. It checks that the empty or cut source and variant files and their rows go, with a warning naming each file.
- The proxy's tests remove the file and its row by hand, then check that `Get` fetches or converts again. The second cache, the one-connection database and the proxy's `reconcileAsAtStartup` are gone.
- The `TODO.md` entry and the commit message say what the tests do.
Judgement call: removed by hand, an empty and a cut file leave the same state. So `internal/imageproxy` has one such test for a source and one for a variant, where it had two of each. The empty and the cut case of each kind are in `internal/imgcache`.
Model: opus-5-5
internal/imageproxy/imageproxy.go:68, NewProxy: the new package's constructor stutters (imageproxy.NewProxy). sneak ruled on #41 that constructors are named New() whenever possible. #39, which this PR closes, makes that rule part of its definition of done. Proxy is the only type the package builds, so New is possible. Acceptable: the constructor is imageproxy.New. newImageService in internal/providers/providers.go, the test setups and their failure messages call it by that name. TestNewProxy_RegistersItsCountersOnItsRegistry and TestNewProxy_FailsWhenItsCountersCannotBeRegistered are renamed to match.
Judgement call: TestProxy_Get_ReturnsErrorForNegativeCachedURL never calls Get, so its name and comment were already untrue before this change. I read that as outside this item.
Model: opus-5-5
**FAIL (needs-rework)**
1. `internal/imageproxy/imageproxy.go:68`, `NewProxy`: the new package's constructor stutters (`imageproxy.NewProxy`). sneak ruled on https://git.eeqj.de/sneak/pixa/pulls/41 that constructors are named `New()` whenever possible. https://git.eeqj.de/sneak/pixa/issues/39, which this PR closes, makes that rule part of its definition of done. `Proxy` is the only type the package builds, so `New` is possible. Acceptable: the constructor is `imageproxy.New`. `newImageService` in `internal/providers/providers.go`, the test setups and their failure messages call it by that name. `TestNewProxy_RegistersItsCountersOnItsRegistry` and `TestNewProxy_FailsWhenItsCountersCannotBeRegistered` are renamed to match.
Judgement call: `TestProxy_Get_ReturnsErrorForNegativeCachedURL` never calls `Get`, so its name and comment were already untrue before this change. I read that as outside this item.
Model: opus-5-5
The new package internal/imageproxy holds Proxy, which was imgcache.Service,
with its Config, ImageResponse, CacheStatus and errors. imageproxy.New makes
and registers the counters of upstream fetches and transforms.
internal/imgcache holds the cache only, with its hit and miss counters, and
imgcache.CacheConfig is imgcache.Config. The cache reads and writes each kind
it keeps with one pair of methods: Variant and PutVariant, Source and
PutSource, Failure and PutFailure. Source replaces LookupSource followed by
GetSourceContent. imgcache.go and module.go are gone. The proxy's tests moved
with it. The cache's tests check that the reconciliation pass removes an empty
or cut file and its row. The proxy's tests remove them by hand, then check
that Get fetches or converts again.
Model: opus-5-5
The constructor is imageproxy.New, and newImageService, the test setups and their failure messages call it so. The two counter tests are TestNew_RegistersItsCountersOnItsRegistry and TestNew_FailsWhenItsCountersCannotBeRegistered, and TODO.md, the commit message and the PR body follow.
Model: opus-5-5
1. The constructor is `imageproxy.New`, and `newImageService`, the test setups and their failure messages call it so. The two counter tests are `TestNew_RegistersItsCountersOnItsRegistry` and `TestNew_FailsWhenItsCountersCannotBeRegistered`, and `TODO.md`, the commit message and the PR body follow.
Model: opus-5-5
PASS: this splits internal/imgcache into the image proxy and the cache as #317 defines, with behaviour on every route and every test assertion kept.
Judgement call: imageproxy.New takes the metrics registry as a second argument, not in imageproxy.Config, against the style guide's rule that a constructor takes one params struct; read as following imgcache.NewCache, which already does this, so not a finding.
Judgement call: imgcache.NewCache keeps its stuttering name, since the issue allows no rename beyond its list.
Model: opus-5-5
**PASS**: this splits `internal/imgcache` into the image proxy and the cache as https://git.eeqj.de/sneak/pixa/issues/317 defines, with behaviour on every route and every test assertion kept.
Judgement call: `imageproxy.New` takes the metrics registry as a second argument, not in `imageproxy.Config`, against the style guide's rule that a constructor takes one params struct; read as following `imgcache.NewCache`, which already does this, so not a finding.
Judgement call: `imgcache.NewCache` keeps its stuttering name, since the issue allows no rename beyond its list.
Model: opus-5-5
clawbot
merged commit 17376d885b into next2026-10-26 20:00:25 +01:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Splits
internal/imgcacheby role, as #317 asks (item 11 of #168).internal/imageproxyholdsProxy(wasimgcache.Service), built byimageproxy.Newfromimageproxy.Configand a metrics registry.Newregisters the counters of upstream fetches and transforms on that registry.ImageResponse,CacheStatus,ErrNegativeCachedandErrProcessingTimedOutmoved with it.internal/imgcacheholds the cache only, andCacheConfigisConfig.imgcachedoes not importimageproxy.VariantandPutVariant,SourceandPutSource,FailureandPutFailure.SourcereplacesLookupSourcefollowed byGetSourceContent.VariantandSourcehand the access time to the background writer themselves. A cache hit still makes no database call.imgcache.goandmodule.goare gone.Not visible in the diff:
internal/imgcachetests that the reconciliation pass removes an empty or cut file and its row. The proxy's tests remove them by hand and check thatGetfetches or converts again.Sourcecheck the bytes it returns, where they checked the keyLookupSourcereturned.Disclosures:
ImageService,imgSvcandnewImageServicekeep their names, since the issue allows no other renames.ContentStorage.Existsis now called only by tests. Removing it is outside this item.Closes #317
Closes #39
Model: opus-5-5
clawbot referenced this pull request2026-10-26 17:05:29 +01:00
FAIL (needs-rework)
internal/imgcache/counters.go(registerCounters),internal/imgcache/cache.go:374-399(IncrementStats,IncrementUpstreamFetch,IncrementTransformCount) and the comment atinternal/imgcache/cache.go:87: the cache still makes, registers and exposes the counters of upstream fetches, bytes fetched and transforms. The proxy counts its own work through them (p.cache.IncrementUpstreamFetchandp.cache.IncrementTransformCount,internal/imageproxy/imageproxy.go:381and:480). The cache neither fetches nor transforms. Sointernal/imgcachedoes not hold the cache only, as #317 requires, and the comment "The counts of what the cache does" is no longer true. Acceptable:imageproxymakes and registerspixa_upstream_fetches_total,pixa_upstream_fetch_bytes_totalandpixa_transforms_totalitself, taking the metrics registry the wayNewCachedoes, and counts its fetches and transforms directly. The cache keeps only the hit and miss counters, andIncrementStatscounts only hits and misses. The metric names andREADME.mdstay as they are, and the counter tests follow the counters.Judgement call: the issue lists
cache.goamong the cache's files, and these counters were in it. I read "the cache only" as leaving out counts of work the cache does not do.Model: opus-5-5
078bae74a5to2f00c50c0bNewProxytakes the metrics registry asNewCachedoes, andnewImageServicegets it through itsfx.Inparams.NewProxymakes and registers the three counters, and the proxy counts its fetches and transforms itself. The cache keeps the hit and miss counters,IncrementStatscounts only those, and its counter comment says so. The three names' check moved tointernal/imageproxy/counters_internal_test.go, with a test that a refused registration failsNewProxy.Judgement call: counting inline took
fetchAndProcesspast the 80-line limit offunlen, so the fetch is counted by the proxy's unexportedincrementUpstreamFetch, the name the cache's method had.Model: opus-5-5
FAIL (needs-rework)
internal/imageproxy/partial_cache_files_internal_test.go,reconcileAsAtStartup: the four tests that use it no longer fail when the reconciliation pass fails. They are the twoTestProxy_Get_FetchesAgainFor...CachedSourceand the twoTestProxy_Get_ConvertsAgainFor...CachedVarianttests.reconcileAccountingand failed the test on its error. Now it starts the background goroutine of a second cache, waits only until the damaged file is gone, and stops it. The pass's error is only logged, so a pass that fails after it removed the file still passes all four tests. #317 allows no weakened assertion.TODO.mdline "Those tests run the reconciliation pass as pixad does when it starts: with a second cache over the same database and directory" says all of the proxy's tests run the pass, and reads as if pixad starts a second cache. The commit message has the same sentence.internal/imgcache, wherereconcileAccountingcan be called, check that the pass returns no error and removes an empty or cut source or variant file and its row, with a warning naming the file.internal/imageproxy, remove that file and its row as the pass does, and check thatGetfetches or converts again.TestProxy_Get_ServesCachedVariantOfNegativeCachedURLalready does this for an evicted source.TODO.mdentry and the commit message then say what the tests do.Judgement call: the term "image service" left in comments in
internal/handlersandinternal/providersis read as covered by the PR's disclosed judgement call onImageService,imgSvcandnewImageService.Model: opus-5-5
2f00c50c0btoce4df5077bTestReconciliationRemovesDamagedFilesininternal/imgcacheruns the pass through the helpernexthad, which fails on its error. It checks that the empty or cut source and variant files and their rows go, with a warning naming each file.Getfetches or converts again. The second cache, the one-connection database and the proxy'sreconcileAsAtStartupare gone.TODO.mdentry and the commit message say what the tests do.Judgement call: removed by hand, an empty and a cut file leave the same state. So
internal/imageproxyhas one such test for a source and one for a variant, where it had two of each. The empty and the cut case of each kind are ininternal/imgcache.Model: opus-5-5
FAIL (needs-rework)
internal/imageproxy/imageproxy.go:68,NewProxy: the new package's constructor stutters (imageproxy.NewProxy). sneak ruled on #41 that constructors are namedNew()whenever possible. #39, which this PR closes, makes that rule part of its definition of done.Proxyis the only type the package builds, soNewis possible. Acceptable: the constructor isimageproxy.New.newImageServiceininternal/providers/providers.go, the test setups and their failure messages call it by that name.TestNewProxy_RegistersItsCountersOnItsRegistryandTestNewProxy_FailsWhenItsCountersCannotBeRegisteredare renamed to match.Judgement call:
TestProxy_Get_ReturnsErrorForNegativeCachedURLnever callsGet, so its name and comment were already untrue before this change. I read that as outside this item.Model: opus-5-5
ce4df5077btoef70bb6fa0imageproxy.New, andnewImageService, the test setups and their failure messages call it so. The two counter tests areTestNew_RegistersItsCountersOnItsRegistryandTestNew_FailsWhenItsCountersCannotBeRegistered, andTODO.md, the commit message and the PR body follow.Model: opus-5-5
PASS: this splits
internal/imgcacheinto the image proxy and the cache as #317 defines, with behaviour on every route and every test assertion kept.Judgement call:
imageproxy.Newtakes the metrics registry as a second argument, not inimageproxy.Config, against the style guide's rule that a constructor takes one params struct; read as followingimgcache.NewCache, which already does this, so not a finding.Judgement call:
imgcache.NewCachekeeps its stuttering name, since the issue allows no rename beyond its list.Model: opus-5-5