Discovered while implementing #51 (out of scope there, filing per repo policy).
Cache.Stats in internal/imgcache/cache.go computes TotalItems from request_cache and TotalSizeBytes from output_content:
_ = c.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM request_cache`).Scan(&stats.TotalItems)
_ = c.db.QueryRowContext(ctx, `SELECT COALESCE(SUM(size_bytes), 0) FROM output_content`).Scan(&stats.TotalSizeBytes)
No code path ever inserts into output_content or request_cache (they were created by migration 001 for a design that stored variants content-addressed with a request mapping, but the implementation went with key-addressed VariantStorage instead). Both stats are therefore always 0.
Since #51 (PR #55), real accounting exists: variant_content tracks processed variants and source_content tracks source blobs, and Cache.UsageBytes returns the true total. Suggested fix:
Point TotalItems/TotalSizeBytes at variant_content + source_content (or reuse UsageBytes).
Decide whether to drop the dead output_content/request_cache tables in a future migration or leave them for the originally intended design.
Also worth a look in the same pass: the metaCache field on Cache (in-memory variant meta cache) is initialized but never read or written.
Discovered while implementing #51 (out of scope there, filing per repo policy).
`Cache.Stats` in `internal/imgcache/cache.go` computes `TotalItems` from `request_cache` and `TotalSizeBytes` from `output_content`:
```
_ = c.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM request_cache`).Scan(&stats.TotalItems)
_ = c.db.QueryRowContext(ctx, `SELECT COALESCE(SUM(size_bytes), 0) FROM output_content`).Scan(&stats.TotalSizeBytes)
```
No code path ever inserts into `output_content` or `request_cache` (they were created by migration 001 for a design that stored variants content-addressed with a request mapping, but the implementation went with key-addressed `VariantStorage` instead). Both stats are therefore always 0.
Since #51 (PR #55), real accounting exists: `variant_content` tracks processed variants and `source_content` tracks source blobs, and `Cache.UsageBytes` returns the true total. Suggested fix:
1. Point `TotalItems`/`TotalSizeBytes` at `variant_content` + `source_content` (or reuse `UsageBytes`).
2. Decide whether to drop the dead `output_content`/`request_cache` tables in a future migration or leave them for the originally intended design.
Also worth a look in the same pass: the `metaCache` field on `Cache` (in-memory variant meta cache) is initialized but never read or written.
Adding a related finding to this issue's scope rather than filing a near-duplicate, found during the 1.0.0 survey and verified against main at 61f42e6.
The stats problem is wider than Cache.Stats reading the wrong tables — the counter columns are never written either:
IncrementStats(ctx, hit, fetchBytes) (internal/imgcache/cache.go:324-344) has a fetchBytes > 0 branch that updates upstream_fetch_bytes, but both call sites pass 0 (internal/imgcache/service.go:134 and :148). So upstream_fetch_count and upstream_fetch_bytes never move.
transform_count has no writer anywhere in the codebase.
So even after this issue repoints TotalItems/TotalSizeBytes at the real accounting tables (variant_content/source_content, which PR #55 introduces and which do have correct writers), the remaining stats columns will still be permanently zero.
Suggest folding into this issue's definition of done, since it is the same "stats surface reports zeros" defect:
IncrementStats receives the real fetched byte count from both call sites in service.go rather than 0.
transform_count is incremented when a transcode actually happens, or the column is dropped if it is not worth maintaining.
A test asserting each counter actually moves — a stats field with no test is how all of these silently became zero in the first place.
Also still open from this issue's original body: the dead metaCache field. That has grown enough scope (it is the mechanism behind README.md:49's 1-5k r/s claim, and it needs bounding plus eviction-invalidation) that I filed it separately rather than leaving it as a footnote here.
Milestoned 1.0.0. Do this after PR #55 merges — it introduces the accounting tables this should read from.
Adding a related finding to this issue's scope rather than filing a near-duplicate, found during the 1.0.0 survey and verified against `main` at `61f42e6`.
The stats problem is wider than `Cache.Stats` reading the wrong tables — **the counter columns are never written either**:
- `IncrementStats(ctx, hit, fetchBytes)` (`internal/imgcache/cache.go:324-344`) has a `fetchBytes > 0` branch that updates `upstream_fetch_bytes`, but **both** call sites pass `0` (`internal/imgcache/service.go:134` and `:148`). So `upstream_fetch_count` and `upstream_fetch_bytes` never move.
- `transform_count` has no writer anywhere in the codebase.
So even after this issue repoints `TotalItems`/`TotalSizeBytes` at the real accounting tables (`variant_content`/`source_content`, which PR #55 introduces and which do have correct writers), the remaining stats columns will still be permanently zero.
Suggest folding into this issue's definition of done, since it is the same "stats surface reports zeros" defect:
1. `IncrementStats` receives the real fetched byte count from both call sites in `service.go` rather than `0`.
2. `transform_count` is incremented when a transcode actually happens, or the column is dropped if it is not worth maintaining.
3. A test asserting each counter actually moves — a stats field with no test is how all of these silently became zero in the first place.
Also still open from this issue's original body: the dead `metaCache` field. That has grown enough scope (it is the mechanism behind `README.md:49`'s 1-5k r/s claim, and it needs bounding plus eviction-invalidation) that I filed it separately rather than leaving it as a footnote here.
Milestoned `1.0.0`. Do this after PR #55 merges — it introduces the accounting tables this should read from.
Cache.Stats: TotalItems becomes the number of rows in source_content plus variant_content, and TotalSizeBytes comes from Cache.UsageBytes. A failed query is still logged at warn and Stats still succeeds. The existing test for those warnings drops request_cache and output_content to force the failure; it will drop source_content and variant_content instead, the tables Stats reads after this change.
Upstream fetch counters: Service.Get counts a miss after the image is produced (or fails), passing the bytes that request fetched from upstream: 0 when the source came from the disk cache, otherwise the length fetched, also when a later step (magic byte check, processing) fails. The hit call site keeps passing 0, because a hit fetches nothing.
transform_count: a new Cache.IncrementTransformCount, called in processAndStore once the image processor returns without error (every such call decodes and re-encodes the image).
Tests, committed first: one for each counter through Service.Get, and one for the Stats totals after storing a source and a variant.
Left alone: the unused output_content and request_cache tables (dropping them is a schema decision), and the unused metaCache field (#70).
Model: opus-5-5
Plan, checked against current `next`:
- `Cache.Stats`: `TotalItems` becomes the number of rows in `source_content` plus `variant_content`, and `TotalSizeBytes` comes from `Cache.UsageBytes`. A failed query is still logged at `warn` and `Stats` still succeeds. The existing test for those warnings drops `request_cache` and `output_content` to force the failure; it will drop `source_content` and `variant_content` instead, the tables `Stats` reads after this change.
- Upstream fetch counters: `Service.Get` counts a miss after the image is produced (or fails), passing the bytes that request fetched from upstream: 0 when the source came from the disk cache, otherwise the length fetched, also when a later step (magic byte check, processing) fails. The hit call site keeps passing 0, because a hit fetches nothing.
- `transform_count`: a new `Cache.IncrementTransformCount`, called in `processAndStore` once the image processor returns without error (every such call decodes and re-encodes the image).
- Tests, committed first: one for each counter through `Service.Get`, and one for the `Stats` totals after storing a source and a variant.
- Left alone: the unused `output_content` and `request_cache` tables (dropping them is a schema decision), and the unused `metaCache` field (https://git.eeqj.de/sneak/pixa/issues/70).
Model: opus-5-5
Built as planned in #144. One difference from the plan: the counters are covered by one test that walks Service.Get through a miss that fetches, a hit, a miss that reuses the cached source and two misses that fail, checking every counter after each step, rather than a separate test per counter.
Model: opus-5-5
Built as planned in https://git.eeqj.de/sneak/pixa/pulls/144. One difference from the plan: the counters are covered by one test that walks `Service.Get` through a miss that fetches, a hit, a miss that reuses the cached source and two misses that fail, checking every counter after each step, rather than a separate test per counter.
Model: opus-5-5
Question for sneak: approve one edit to an existing test in #144? This repo's rules require your approval before an existing test is changed.
TestStats_LogsFailedCountQueries (added today by #72) makes the two Stats count queries fail by dropping the tables they read, then checks a warning is logged. This issue points Stats at the tables that actually hold the cache (source_content, variant_content) instead of two tables nothing writes, so the test now drops those two instead. Its assertions are unchanged.
Options: (1) approve (recommended; the test still checks the same thing, against the tables Stats now reads); (2) refuse, and the test keeps dropping tables Stats no longer reads, so it fails and the fix cannot land.
Until you answer, the PR goes through review but is not merged; other work carries on.
Model: opus-5-5
Question for sneak: approve one edit to an existing test in https://git.eeqj.de/sneak/pixa/pulls/144? This repo's rules require your approval before an existing test is changed.
`TestStats_LogsFailedCountQueries` (added today by https://git.eeqj.de/sneak/pixa/issues/72) makes the two `Stats` count queries fail by dropping the tables they read, then checks a warning is logged. This issue points `Stats` at the tables that actually hold the cache (`source_content`, `variant_content`) instead of two tables nothing writes, so the test now drops those two instead. Its assertions are unchanged.
Options: (1) approve (recommended; the test still checks the same thing, against the tables `Stats` now reads); (2) refuse, and the test keeps dropping tables `Stats` no longer reads, so it fails and the fix cannot land.
Until you answer, the PR goes through review but is not merged; other work carries on.
Model: opus-5-5
sneak
was assigned by clawbot2026-09-29 02:00:42 +02: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.
Discovered while implementing #51 (out of scope there, filing per repo policy).
Cache.Statsininternal/imgcache/cache.gocomputesTotalItemsfromrequest_cacheandTotalSizeBytesfromoutput_content:No code path ever inserts into
output_contentorrequest_cache(they were created by migration 001 for a design that stored variants content-addressed with a request mapping, but the implementation went with key-addressedVariantStorageinstead). Both stats are therefore always 0.Since #51 (PR #55), real accounting exists:
variant_contenttracks processed variants andsource_contenttracks source blobs, andCache.UsageBytesreturns the true total. Suggested fix:TotalItems/TotalSizeBytesatvariant_content+source_content(or reuseUsageBytes).output_content/request_cachetables in a future migration or leave them for the originally intended design.Also worth a look in the same pass: the
metaCachefield onCache(in-memory variant meta cache) is initialized but never read or written.Adding a related finding to this issue's scope rather than filing a near-duplicate, found during the 1.0.0 survey and verified against
mainat61f42e6.The stats problem is wider than
Cache.Statsreading the wrong tables — the counter columns are never written either:IncrementStats(ctx, hit, fetchBytes)(internal/imgcache/cache.go:324-344) has afetchBytes > 0branch that updatesupstream_fetch_bytes, but both call sites pass0(internal/imgcache/service.go:134and:148). Soupstream_fetch_countandupstream_fetch_bytesnever move.transform_counthas no writer anywhere in the codebase.So even after this issue repoints
TotalItems/TotalSizeBytesat the real accounting tables (variant_content/source_content, which PR #55 introduces and which do have correct writers), the remaining stats columns will still be permanently zero.Suggest folding into this issue's definition of done, since it is the same "stats surface reports zeros" defect:
IncrementStatsreceives the real fetched byte count from both call sites inservice.gorather than0.transform_countis incremented when a transcode actually happens, or the column is dropped if it is not worth maintaining.Also still open from this issue's original body: the dead
metaCachefield. That has grown enough scope (it is the mechanism behindREADME.md:49's 1-5k r/s claim, and it needs bounding plus eviction-invalidation) that I filed it separately rather than leaving it as a footnote here.Milestoned
1.0.0. Do this after PR #55 merges — it introduces the accounting tables this should read from.Plan, checked against current
next:Cache.Stats:TotalItemsbecomes the number of rows insource_contentplusvariant_content, andTotalSizeBytescomes fromCache.UsageBytes. A failed query is still logged atwarnandStatsstill succeeds. The existing test for those warnings dropsrequest_cacheandoutput_contentto force the failure; it will dropsource_contentandvariant_contentinstead, the tablesStatsreads after this change.Service.Getcounts a miss after the image is produced (or fails), passing the bytes that request fetched from upstream: 0 when the source came from the disk cache, otherwise the length fetched, also when a later step (magic byte check, processing) fails. The hit call site keeps passing 0, because a hit fetches nothing.transform_count: a newCache.IncrementTransformCount, called inprocessAndStoreonce the image processor returns without error (every such call decodes and re-encodes the image).Service.Get, and one for theStatstotals after storing a source and a variant.output_contentandrequest_cachetables (dropping them is a schema decision), and the unusedmetaCachefield (#70).Model: opus-5-5
Built as planned in #144. One difference from the plan: the counters are covered by one test that walks
Service.Getthrough a miss that fetches, a hit, a miss that reuses the cached source and two misses that fail, checking every counter after each step, rather than a separate test per counter.Model: opus-5-5
Question for sneak: approve one edit to an existing test in #144? This repo's rules require your approval before an existing test is changed.
TestStats_LogsFailedCountQueries(added today by #72) makes the twoStatscount queries fail by dropping the tables they read, then checks a warning is logged. This issue pointsStatsat the tables that actually hold the cache (source_content,variant_content) instead of two tables nothing writes, so the test now drops those two instead. Its assertions are unchanged.Options: (1) approve (recommended; the test still checks the same thing, against the tables
Statsnow reads); (2) refuse, and the test keeps dropping tablesStatsno longer reads, so it fails and the fix cannot land.Until you answer, the PR goes through review but is not merged; other work carries on.
Model: opus-5-5
approved
clawbot referenced this issue2026-09-29 05:25:41 +02:00
clawbot referenced this issue2026-10-08 14:30:59 +02:00