feat: cache size management and LRU eviction (closes #51) #55

clawbot · 2026-08-07T23:12:46+02:00

clawbot commented

2026-08-07 23:12:46 +02:00

Implements #51 per the issue DoD and the owner direction comment (issuecomment-44068).

Behavior

Config: cache_max_bytes (integrates with the #52/#53 validation framework)

Strict int64 parsing via a new getInt64/int64Val getter in the existing strict-loader pattern; the key is registered in the known-keys list. A SET but invalid value — negative, float, null, non-numeric string, boolean, list — aborts startup with exit 1 naming the key and the offending value.
Explicit values are used exactly as given, any non-negative amount, no floor. cache_max_bytes: 0 is a valid value that disables the disk cache entirely.
Omitted: after state_dir validation, the default resolves to max(75% of free bytes on the filesystem containing <state_dir>/cache/, 500 MiB). The cache directory is created first and statfs runs on that actual path, so the measurement hits the right filesystem. The probe is injectable (FreeSpaceProbeFunc) so tests do not depend on the host disk. The effective limit (and disabled state) is logged at startup.

Size accounting (no directory scans on the hot path)

Migration 002 adds a variant_content table — processed variants were previously untracked anywhere — and a last_accessed_at column on source_content, both indexed. Total usage is two SUM queries.
Stores record accounting rows; cache hits touch the LRU timestamps (same cost class as the existing per-request stats UPDATEs). The variant accounting insert is best-effort with a warning: the reconciliation pass (below) adopts any file that missed its row, and this keeps the pre-migration inline test schema working.

Eviction policy: global LRU across both content classes

Candidates are the least-recently-used entries from variant_content and source_content (batched, 100 per class per pass, merged oldest-first by COALESCE(last_accessed_at, fetched_at)), evicted until usage is at or below the limit.
Why global LRU: recency of actual use is the best cheap predictor of future use for a CDN-style cache, and treating both classes in one ordering avoids pathologies of class-priority schemes (e.g. evicting every variant before any cold source blob, which would tank hit rate, or the reverse, which would hoard stale sources). Byte-for-byte, the coldest data goes first regardless of what kind it is. LFU-style schemes need more bookkeeping for marginal gain at this scale.
Reference safety (the multi-reference DoD case): evicting a source blob deletes ALL source_metadata rows referencing it plus its source_content row in a single transaction BEFORE the file is unlinked. A blob referenced by multiple source paths is only ever removed together with all of its references, and DB rows never point at deleted files (the crash window leaves at worst an orphaned file, which reconciliation sweeps). The JSON metadata sidecars for removed rows are deleted as well.

Triggers, off the request path

A background goroutine (started in the handlers OnStart hook, stopped in OnStop) runs an eviction pass on a periodic ticker (5 min) and on write pressure: every store sends a non-blocking notification on a capacity-1 channel. Requests never wait on eviction.
On startup the goroutine first reconciles accounting with the disk (off the hot path): adopts untracked variant files (size/mtime from disk, content type from the .meta sidecar), drops accounting rows whose files are missing, removes source blob files the DB does not know (unreachable, since lookups go through source_metadata), removes rows whose files are gone, and sweeps .tmp-* files older than an hour.

cache_max_bytes: 0 disables the disk cache

No cache directories are created, lookups always miss, StoreSource/StoreVariant are no-ops, no evictor runs; every request fetches and processes uncached. Verified end-to-end (below).

Notes for review

At the imgcache.CacheConfig layer, disabling is an explicit DisableDiskCache flag rather than MaxBytes == 0, because existing test fixtures construct CacheConfig without MaxBytes and rely on the legacy "no limit" behavior; per repo rules those tests were not touched. The config layer maps cache_max_bytes: 0 to the flag in handlers. MaxBytes == 0 at that layer means "no limit enforced" and is unreachable from production config (the computed default is always at least 500 MiB).
The negative cache stays active in disabled mode: it is DB-backed (TTL-expired rows in SQLite), not part of the disk cache this issue bounds, and it protects against hammering failing upstreams. Flagging explicitly since the direction said "no cache reads, no cache writes" — I read that as the disk cache; happy to disable it too if intended.
One deviation from pure red/green: after the red commit I extended the new-test fixture helper (newEvictionTestCache) to pass DisableDiskCache: maxBytes == 0, mirroring the production mapping, when the flag design emerged. Assertions were not touched; no pre-existing tests were modified.
Sidecar files (.meta, metadata JSON) are not counted in usage; they are bounded by entry counts and small (tens of bytes to ~1 KiB per entry) while content bytes dominate. Documented here for transparency.
Discovered while working: Cache.Stats reads the never-populated output_content/request_cache tables, so TotalItems/TotalSizeBytes are always 0. Out of scope here; filing as a separate issue.

Verification

TDD: commit 3963ec3 adds the failing tests first (18 new tests covering strict parsing, default computation with injected probe including floor and 75% branches, explicit-no-floor, zero-disables, size accounting, dedup accounting, LRU order, multi-reference blob eviction with the no-dangling-references invariant, under-limit no-op, write-pressure trigger, periodic trigger, reconciliation); implementation follows in 8cb09b6/bdd86a4 until green.
make check green (all tests, lint 0 issues, fmt-check) at HEAD.
Pinned CI lint gate: docker build --target lint . green (golangci-lint v2.10.1).
End-to-end with the built binary:
- omitted key: startup logs computed default cache size limit from free space and effective cache size limit (75% of the test host's free space);
- cache_max_bytes: banana: exit 1 with config key "cache_max_bytes": value "banana" is not an integer;
- cache_max_bytes: 0: cache_disabled=true logged, two identical requests both fetch upstream (2 upstream fetches logged), 200 image/jpeg responses, no cache/ directory created, only state.sqlite3 in the state dir;
- enabled: second request served from cache (1 upstream fetch), variant_content and source_content rows match the on-disk file sizes.

Implements #51 per the issue DoD and the owner direction comment (issuecomment-44068). ## Behavior **Config: `cache_max_bytes`** (integrates with the #52/#53 validation framework) - Strict int64 parsing via a new `getInt64`/`int64Val` getter in the existing strict-loader pattern; the key is registered in the known-keys list. A SET but invalid value — negative, float, null, non-numeric string, boolean, list — aborts startup with exit 1 naming the key and the offending value. - Explicit values are used exactly as given, any non-negative amount, no floor. `cache_max_bytes: 0` is a valid value that disables the disk cache entirely. - Omitted: after `state_dir` validation, the default resolves to `max(75% of free bytes on the filesystem containing <state_dir>/cache/, 500 MiB)`. The cache directory is created first and statfs runs on that actual path, so the measurement hits the right filesystem. The probe is injectable (`FreeSpaceProbeFunc`) so tests do not depend on the host disk. The effective limit (and disabled state) is logged at startup. **Size accounting** (no directory scans on the hot path) - Migration `002` adds a `variant_content` table — processed variants were previously untracked anywhere — and a `last_accessed_at` column on `source_content`, both indexed. Total usage is two SUM queries. - Stores record accounting rows; cache hits touch the LRU timestamps (same cost class as the existing per-request stats UPDATEs). The variant accounting insert is best-effort with a warning: the reconciliation pass (below) adopts any file that missed its row, and this keeps the pre-migration inline test schema working. **Eviction policy: global LRU across both content classes** - Candidates are the least-recently-used entries from `variant_content` and `source_content` (batched, 100 per class per pass, merged oldest-first by `COALESCE(last_accessed_at, fetched_at)`), evicted until usage is at or below the limit. - Why global LRU: recency of actual use is the best cheap predictor of future use for a CDN-style cache, and treating both classes in one ordering avoids pathologies of class-priority schemes (e.g. evicting every variant before any cold source blob, which would tank hit rate, or the reverse, which would hoard stale sources). Byte-for-byte, the coldest data goes first regardless of what kind it is. LFU-style schemes need more bookkeeping for marginal gain at this scale. - Reference safety (the multi-reference DoD case): evicting a source blob deletes ALL `source_metadata` rows referencing it plus its `source_content` row in a single transaction BEFORE the file is unlinked. A blob referenced by multiple source paths is only ever removed together with all of its references, and DB rows never point at deleted files (the crash window leaves at worst an orphaned file, which reconciliation sweeps). The JSON metadata sidecars for removed rows are deleted as well. **Triggers, off the request path** - A background goroutine (started in the handlers OnStart hook, stopped in OnStop) runs an eviction pass on a periodic ticker (5 min) and on write pressure: every store sends a non-blocking notification on a capacity-1 channel. Requests never wait on eviction. - On startup the goroutine first reconciles accounting with the disk (off the hot path): adopts untracked variant files (size/mtime from disk, content type from the `.meta` sidecar), drops accounting rows whose files are missing, removes source blob files the DB does not know (unreachable, since lookups go through `source_metadata`), removes rows whose files are gone, and sweeps `.tmp-*` files older than an hour. **`cache_max_bytes: 0` disables the disk cache** - No cache directories are created, lookups always miss, `StoreSource`/`StoreVariant` are no-ops, no evictor runs; every request fetches and processes uncached. Verified end-to-end (below). ## Notes for review - At the `imgcache.CacheConfig` layer, disabling is an explicit `DisableDiskCache` flag rather than `MaxBytes == 0`, because existing test fixtures construct `CacheConfig` without `MaxBytes` and rely on the legacy "no limit" behavior; per repo rules those tests were not touched. The config layer maps `cache_max_bytes: 0` to the flag in `handlers`. `MaxBytes == 0` at that layer means "no limit enforced" and is unreachable from production config (the computed default is always at least 500 MiB). - The negative cache stays active in disabled mode: it is DB-backed (TTL-expired rows in SQLite), not part of the disk cache this issue bounds, and it protects against hammering failing upstreams. Flagging explicitly since the direction said "no cache reads, no cache writes" — I read that as the disk cache; happy to disable it too if intended. - One deviation from pure red/green: after the red commit I extended the new-test fixture helper (`newEvictionTestCache`) to pass `DisableDiskCache: maxBytes == 0`, mirroring the production mapping, when the flag design emerged. Assertions were not touched; no pre-existing tests were modified. - Sidecar files (`.meta`, metadata JSON) are not counted in usage; they are bounded by entry counts and small (tens of bytes to ~1 KiB per entry) while content bytes dominate. Documented here for transparency. - Discovered while working: `Cache.Stats` reads the never-populated `output_content`/`request_cache` tables, so `TotalItems`/`TotalSizeBytes` are always 0. Out of scope here; filing as a separate issue. ## Verification - TDD: commit `3963ec3` adds the failing tests first (18 new tests covering strict parsing, default computation with injected probe including floor and 75% branches, explicit-no-floor, zero-disables, size accounting, dedup accounting, LRU order, multi-reference blob eviction with the no-dangling-references invariant, under-limit no-op, write-pressure trigger, periodic trigger, reconciliation); implementation follows in `8cb09b6`/`bdd86a4` until green. - `make check` green (all tests, lint 0 issues, fmt-check) at HEAD. - Pinned CI lint gate: `docker build --target lint .` green (golangci-lint v2.10.1). - End-to-end with the built binary: - omitted key: startup logs `computed default cache size limit from free space` and `effective cache size limit` (75% of the test host's free space); - `cache_max_bytes: banana`: exit 1 with `config key "cache_max_bytes": value "banana" is not an integer`; - `cache_max_bytes: 0`: `cache_disabled=true` logged, two identical requests both fetch upstream (2 upstream fetches logged), 200 `image/jpeg` responses, no `cache/` directory created, only `state.sqlite3` in the state dir; - enabled: second request served from cache (1 upstream fetch), `variant_content` and `source_content` rows match the on-disk file sizes.

clawbot added 4 commits 2026-08-07 23:12:47 +02:00

test: add failing tests for cache_max_bytes config and cache eviction 3963ec31c1

Red phase for #51: covers strict cache_max_bytes parsing (invalid
explicit values abort naming key and value), the computed default of
max(75% of free space, 500 MiB) via an injectable free-space probe,
explicit-value-no-floor, zero-disables-cache, size accounting over
source blobs and variants, LRU eviction under the limit, the
multi-referenced blob case, write-pressure and periodic eviction
triggers, and startup reconciliation. Minimal API skeletons keep the
tree compiling and lint-clean; only the new tests fail.

feat: add cache_max_bytes config key with statfs-derived default 8cb09b6aaf

Strict int64 parsing via the startup validation framework: a SET but
invalid value (negative, float, null, non-numeric) aborts startup
naming the key and value. An omitted key resolves after state_dir
validation to max(75% of free bytes on the filesystem containing
<state_dir>/cache/, 500 MiB), measured via an injectable statfs probe;
the floor never applies to explicit values. Zero is valid and means
the disk cache is disabled. The effective limit is logged at startup.

feat: DB-tracked cache size accounting with background LRU eviction bdd86a4c1e

Migration 002 adds a variant_content table (processed variants were
untracked on disk) and an LRU timestamp on source_content. Total usage
is two SUMs, never a directory scan on the hot path; hits touch LRU
timestamps best-effort. A background goroutine evicts globally
least-recently-used entries (variants and source blobs merged) until
usage is under MaxBytes, woken by a periodic ticker and by non-blocking
write-pressure notifications from stores. Evicting a source blob
deletes all source_metadata rows referencing it plus its
source_content row in one transaction before the file is unlinked, so
multi-referenced blobs are removed only with all their references and
rows never point at deleted files; JSON sidecars are cleaned up too. A
one-time startup reconciliation walk adopts untracked variant files,
drops rows whose files are missing, removes unreachable source blobs,
and sweeps stale temp files. CacheConfig.DisableDiskCache turns the
disk cache off entirely (config maps cache_max_bytes: 0 to it): no
directories, lookups miss, stores no-op, no evictor. Handlers wire the
limit, start eviction on startup, and stop it on shutdown.

docs: document cache_max_bytes, update TODO.md (closes #51 )

check / check (push) Successful in 1m40s

Details

c1ec038c99

Add cache_max_bytes to config.example.yml and the README key settings
list. TODO.md: move cache size management and eviction to Completed
Steps, promote P1 blocked networks configuration into Next Step, and
note in Status that the unbounded disk growth DoS vector is closed.

clawbot added the needs-review label 2026-08-07 23:12:51 +02:00

clawbot self-assigned this 2026-08-07 23:12:52 +02:00

clawbot referenced this pull request

2026-08-07 23:13:03 +02:00

Cache.Stats reads never-populated output_content/request_cache tables, so TotalItems and TotalSizeBytes are always 0 #56

clawbot commented

2026-08-07 23:13:15 +02:00

Built and verified as described in the PR body. Summary of what was done and how it was checked:

Commits (branch feature/cache-size-eviction from main at 61f42e6, head c1ec038):

3963ec3 — red phase: 18 failing tests (config parsing/default/floor rules, size accounting, LRU eviction, multi-reference blob safety, zero-disables, write-pressure and periodic triggers, reconciliation) plus minimal API skeletons so the tree compiles and lints; verified at that commit that ONLY the new tests failed.
8cb09b6 — cache_max_bytes config key: strict getInt64 getter, known-keys registration, non-negative validation, statfs-derived default with injectable probe, effective-limit logging.
bdd86a4 — migration 002 (variant_content + source_content.last_accessed_at), usage accounting, global-LRU background evictor with write-pressure and periodic triggers, transactional reference-safe source blob eviction, startup reconciliation, DisableDiskCache mode, handlers wiring (start on OnStart, stop on OnStop).
c1ec038 — docs (config.example.yml, README key list) and TODO.md Workflow bookkeeping (P1 blocked networks promoted to Next Step).

Verification: make check green at HEAD (all tests, golangci-lint 0 issues, fmt-check); docker build --target lint . green against the pinned CI golangci-lint v2.10.1; end-to-end runs of the built pixad confirming the computed default is logged, an invalid cache_max_bytes exits 1 naming key and value, cache_max_bytes: 0 serves every request uncached with no cache directory created, and the enabled path serves the second request from cache with accounting rows matching on-disk sizes.

Discovered issue filed separately: #56 (Cache.Stats reads the never-populated output_content/request_cache tables).

Built and verified as described in the PR body. Summary of what was done and how it was checked: **Commits** (branch `feature/cache-size-eviction` from `main` at `61f42e6`, head `c1ec038`): - `3963ec3` — red phase: 18 failing tests (config parsing/default/floor rules, size accounting, LRU eviction, multi-reference blob safety, zero-disables, write-pressure and periodic triggers, reconciliation) plus minimal API skeletons so the tree compiles and lints; verified at that commit that ONLY the new tests failed. - `8cb09b6` — `cache_max_bytes` config key: strict `getInt64` getter, known-keys registration, non-negative validation, statfs-derived default with injectable probe, effective-limit logging. - `bdd86a4` — migration 002 (`variant_content` + `source_content.last_accessed_at`), usage accounting, global-LRU background evictor with write-pressure and periodic triggers, transactional reference-safe source blob eviction, startup reconciliation, `DisableDiskCache` mode, handlers wiring (start on OnStart, stop on OnStop). - `c1ec038` — docs (`config.example.yml`, README key list) and `TODO.md` Workflow bookkeeping (P1 blocked networks promoted to Next Step). **Verification**: `make check` green at HEAD (all tests, golangci-lint 0 issues, fmt-check); `docker build --target lint .` green against the pinned CI golangci-lint v2.10.1; end-to-end runs of the built `pixad` confirming the computed default is logged, an invalid `cache_max_bytes` exits 1 naming key and value, `cache_max_bytes: 0` serves every request uncached with no cache directory created, and the enabled path serves the second request from cache with accounting rows matching on-disk sizes. Discovered issue filed separately: #56 (`Cache.Stats` reads the never-populated `output_content`/`request_cache` tables).

clawbot referenced this pull request

2026-08-07 23:13:22 +02:00

P0: implement cache size management and eviction #51

check / check (push) Successful in 1m40s

Details

Check / check (pull_request)

Required

Some required checks are missing.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin feature/cache-size-eviction:feature/cache-size-eviction

git checkout feature/cache-size-eviction

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#55