P0: implement cache size management and eviction #51

New Issue

clawbot · 2026-08-07T18:06:42+02:00

clawbot commented

2026-08-07 18:06:42 +02:00

The disk-backed caches (src-content, src-metadata, dst-content under <statedir>/cache/) grow without bound; a busy or abusive workload can fill the disk. This is the promoted Next Step in TODO.md (after the #49 manual test pass) and a production P0: unbounded disk growth is a denial-of-service vector.

Definition of done

A configurable maximum cache size (e.g. cache_max_bytes in the YAML config) with a sane default; the config key naming must follow the existing descriptive-naming conventions.
An eviction policy that keeps total cache usage under the limit (LRU or similar; document the choice and why in the PR). Eviction must respect the database's content-reference tracking — a content blob referenced by multiple source paths must not be deleted while still referenced, and DB rows must not point at deleted files.
Eviction runs both on a periodic schedule and under write pressure, without blocking request serving (no full-directory scans on the hot path).
TDD per repo rules: failing tests first for size accounting and the eviction policy (including the multi-reference blob case), committed before the implementation.
make check green; TODO.md Workflow bookkeeping in the finishing commit (promote the next Future Step: config validation on startup); finishing commit title ends with (closes #N) for this issue.

Process

Standard workflow: feature branch from main, implementor works from direction comments on this issue, discussion on the PR, adversarial review, merge-ready + assign sneak on pass.

The disk-backed caches (`src-content`, `src-metadata`, `dst-content` under `<statedir>/cache/`) grow without bound; a busy or abusive workload can fill the disk. This is the promoted Next Step in `TODO.md` (after the #49 manual test pass) and a production P0: unbounded disk growth is a denial-of-service vector. ## Definition of done 1. A configurable maximum cache size (e.g. `cache_max_bytes` in the YAML config) with a sane default; the config key naming must follow the existing descriptive-naming conventions. 2. An eviction policy that keeps total cache usage under the limit (LRU or similar; document the choice and why in the PR). Eviction must respect the database's content-reference tracking — a content blob referenced by multiple source paths must not be deleted while still referenced, and DB rows must not point at deleted files. 3. Eviction runs both on a periodic schedule and under write pressure, without blocking request serving (no full-directory scans on the hot path). 4. TDD per repo rules: failing tests first for size accounting and the eviction policy (including the multi-reference blob case), committed before the implementation. 5. `make check` green; `TODO.md` Workflow bookkeeping in the finishing commit (promote the next Future Step: config validation on startup); finishing commit title ends with ` (closes #N)` for this issue. ## Process Standard workflow: feature branch from `main`, implementor works from direction comments on this issue, discussion on the PR, adversarial review, `merge-ready` + assign sneak on pass.

clawbot referenced this issue

2026-08-07 18:06:54 +02:00

P0: validate configuration on startup, fail fast on bad config #52

clawbot referenced this issue

2026-08-07 18:06:58 +02:00

µPaaS deployment setup #17

clawbot referenced this issue

2026-08-07 18:26:17 +02:00

docs: record manual test pass of auth and encrypted URL flows (closes #49) #50

clawbot commented

2026-08-07 22:42:51 +02:00

Owner direction received (supplements the definition of done above; where they differ, this comment wins):

Config key: cache_max_bytes, a non-negative integer number of bytes. Validated by the startup validation framework from #52: a SET but invalid value (negative, non-integer, float, null, non-numeric string) aborts startup naming the key and value. The user may set it to ANY non-negative amount — no floor is applied to explicit values.
Default (key omitted): 75% of the free space of the filesystem containing the cache directory (<state_dir>/cache/), measured at startup via statfs on the actual path, with a floor of 500 MiB (524288000 bytes) — i.e. max(0.75 * free_bytes, 500 MiB). The floor applies ONLY to the computed default, never to explicit values. Log the computed effective limit at startup.
cache_max_bytes: 0 disables the disk cache entirely: no cache reads, no cache writes, no eviction machinery; every request fetches and processes uncached. This is an explicit valid value, not an error.
Tests first (owner re-emphasized): failing tests committed before the implementation, per repo TDD rules — covering size accounting, eviction under the limit (including the multi-reference blob case from the DoD), the 75%/500MiB default computation (injectable free-space probe so tests do not depend on the host disk), explicit-value-no-floor, and the 0-disables-cache path.
TODO.md bookkeeping correction to DoD item 5: config validation on startup is already done (#52/#53, merged). This work is the current Next Step; on completion promote the next Future Step (P1 blocked networks configuration) into Next Step per the Workflow section.

Standard loop applies: feature branch from current main, PR labeled needs-review, adversarial review, merge-ready + assign sneak on pass. Dispatching an implementer now.

Owner direction received (supplements the definition of done above; where they differ, this comment wins): 1. **Config key**: `cache_max_bytes`, a non-negative integer number of bytes. Validated by the startup validation framework from #52: a SET but invalid value (negative, non-integer, float, null, non-numeric string) aborts startup naming the key and value. The user may set it to ANY non-negative amount — no floor is applied to explicit values. 2. **Default (key omitted)**: 75% of the free space of the filesystem containing the cache directory (`<state_dir>/cache/`), measured at startup via statfs on the actual path, with a floor of 500 MiB (524288000 bytes) — i.e. `max(0.75 * free_bytes, 500 MiB)`. The floor applies ONLY to the computed default, never to explicit values. Log the computed effective limit at startup. 3. **`cache_max_bytes: 0` disables the disk cache entirely**: no cache reads, no cache writes, no eviction machinery; every request fetches and processes uncached. This is an explicit valid value, not an error. 4. **Tests first** (owner re-emphasized): failing tests committed before the implementation, per repo TDD rules — covering size accounting, eviction under the limit (including the multi-reference blob case from the DoD), the 75%/500MiB default computation (injectable free-space probe so tests do not depend on the host disk), explicit-value-no-floor, and the 0-disables-cache path. 5. `TODO.md` bookkeeping correction to DoD item 5: config validation on startup is already done (#52/#53, merged). This work is the current Next Step; on completion promote the next Future Step (P1 blocked networks configuration) into Next Step per the Workflow section. Standard loop applies: feature branch from current `main`, PR labeled `needs-review`, adversarial review, `merge-ready` + assign sneak on pass. Dispatching an implementer now.

clawbot commented

2026-08-07 22:47:32 +02:00

Implementation plan (branch feature/cache-size-eviction from main at 61f42e6):

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

New key cache_max_bytes (Config field CacheMaxBytes int64), added to the known-keys list. Strict getInt64 getter: SET-but-invalid values (negative, float, null, non-numeric string) abort startup naming the key and value. Explicit values take any non-negative amount, no floor. 0 is valid and disables the disk cache entirely.
Omitted: default computed after state_dir validation as max(75% of free bytes on the filesystem containing <state_dir>/cache/, 500 MiB), measured via statfs on the actual cache directory (created first). Free-space probe is injectable so tests do not depend on the host disk. Effective limit logged at startup.

Accounting and eviction (in internal/imgcache)

Migration 002: new variant_content table (cache_key PK, size_bytes, content_type, created_at, last_accessed_at) so processed variants are DB-tracked like source blobs already are, plus a last_accessed_at column on source_content; indexes on the LRU timestamps. Total usage = SUM over both tables — no directory scans on the hot path.
Cache hits touch last_accessed_at (variants on lookup, source blobs on source reuse), same cost class as the existing per-request stats UPDATEs.
Policy: global LRU across variants and source blobs, ordered by COALESCE(last_accessed_at, created_at/fetched_at), evicted in batches until usage is under the limit. Rationale in the PR body.
Reference safety: deleting a source blob deletes ALL source_metadata rows referencing it (plus their JSON sidecar files) and the source_content row in one transaction BEFORE the file is unlinked — a multi-referenced blob is only ever removed together with all its references, and DB rows never point at deleted files.
Triggers: background evictor goroutine woken by (a) a periodic ticker and (b) a non-blocking write-pressure notification after each store. Requests never block on eviction. A one-time startup reconciliation walk (background, off the hot path) syncs the DB accounting with the actual disk contents (adopts pre-existing untracked variant files, drops rows whose files are missing, removes orphaned blob files and stale temp files).
cache_max_bytes: 0: Cache is constructed disabled — no cache directories, lookups always miss, stores and source lookups are no-ops, no evictor. Every request fetches and processes uncached. Note: the negative cache stays active; it is DB-backed (bounded, TTL-expired rows), not part of the disk cache this issue bounds — will flag this for review in the PR.

Process

TDD: commit 1 is the red phase — failing tests (with minimal API skeletons so the tree compiles) covering size accounting, LRU eviction under the limit, the multi-reference blob case, default computation via injected probe (floor and 75% branches), explicit-value-no-floor, and zero-disables-cache. Implementation follows until make check is green; no existing tests modified.
Docs: config.example.yml and README config list get cache_max_bytes; TODO.md Workflow bookkeeping (promote P1 blocked networks into Next Step) in the finishing commit, title ending (closes #51).
Verification: make check, docker build --target lint ., plus an end-to-end run of the built binary checking the logged default limit, uncached serving with cache_max_bytes: 0, and exit 1 naming the key on an invalid value.

Implementation plan (branch `feature/cache-size-eviction` from `main` at `61f42e6`): **Config** (integrates with the #52/#53 validation framework) - New key `cache_max_bytes` (Config field `CacheMaxBytes int64`), added to the known-keys list. Strict `getInt64` getter: SET-but-invalid values (negative, float, null, non-numeric string) abort startup naming the key and value. Explicit values take any non-negative amount, no floor. `0` is valid and disables the disk cache entirely. - Omitted: default computed after `state_dir` validation as `max(75% of free bytes on the filesystem containing <state_dir>/cache/, 500 MiB)`, measured via statfs on the actual cache directory (created first). Free-space probe is injectable so tests do not depend on the host disk. Effective limit logged at startup. **Accounting and eviction** (in `internal/imgcache`) - Migration `002`: new `variant_content` table (cache_key PK, size_bytes, content_type, created_at, last_accessed_at) so processed variants are DB-tracked like source blobs already are, plus a `last_accessed_at` column on `source_content`; indexes on the LRU timestamps. Total usage = SUM over both tables — no directory scans on the hot path. - Cache hits touch `last_accessed_at` (variants on lookup, source blobs on source reuse), same cost class as the existing per-request stats UPDATEs. - Policy: global LRU across variants and source blobs, ordered by `COALESCE(last_accessed_at, created_at/fetched_at)`, evicted in batches until usage is under the limit. Rationale in the PR body. - Reference safety: deleting a source blob deletes ALL `source_metadata` rows referencing it (plus their JSON sidecar files) and the `source_content` row in one transaction BEFORE the file is unlinked — a multi-referenced blob is only ever removed together with all its references, and DB rows never point at deleted files. - Triggers: background evictor goroutine woken by (a) a periodic ticker and (b) a non-blocking write-pressure notification after each store. Requests never block on eviction. A one-time startup reconciliation walk (background, off the hot path) syncs the DB accounting with the actual disk contents (adopts pre-existing untracked variant files, drops rows whose files are missing, removes orphaned blob files and stale temp files). - `cache_max_bytes: 0`: Cache is constructed disabled — no cache directories, lookups always miss, stores and source lookups are no-ops, no evictor. Every request fetches and processes uncached. Note: the negative cache stays active; it is DB-backed (bounded, TTL-expired rows), not part of the disk cache this issue bounds — will flag this for review in the PR. **Process** - TDD: commit 1 is the red phase — failing tests (with minimal API skeletons so the tree compiles) covering size accounting, LRU eviction under the limit, the multi-reference blob case, default computation via injected probe (floor and 75% branches), explicit-value-no-floor, and zero-disables-cache. Implementation follows until `make check` is green; no existing tests modified. - Docs: `config.example.yml` and README config list get `cache_max_bytes`; `TODO.md` Workflow bookkeeping (promote P1 blocked networks into Next Step) in the finishing commit, title ending ` (closes #51)`. - Verification: `make check`, `docker build --target lint .`, plus an end-to-end run of the built binary checking the logged default limit, uncached serving with `cache_max_bytes: 0`, and exit 1 naming the key on an invalid value.

clawbot referenced this issue from a commit

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

test: add failing tests for cache_max_bytes config and cache eviction

clawbot referenced this issue from a commit

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

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

clawbot referenced a pull request that will close this issue

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

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

clawbot referenced this issue

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:22 +02:00

Implementation is up as PR #55 (#55), labeled needs-review and assigned to clawbot for the review loop. It follows the plan comment above; deviations and judgment calls, all detailed in the PR body:

Cache-layer disable encoding: at the imgcache.CacheConfig layer, disabling is an explicit DisableDiskCache flag instead of MaxBytes == 0, because pre-existing test fixtures construct CacheConfig with a zero MaxBytes and depend on the legacy no-limit behavior (repo rules forbid touching existing tests). The config layer maps cache_max_bytes: 0 to the flag; observable behavior matches the direction exactly (verified end-to-end).
Negative cache stays active when the disk cache is disabled — it is DB-backed and TTL-bounded, not part of the disk cache this issue bounds; flagged in the PR for an explicit call if it should be disabled too.
Variant accounting inserts are best-effort (warn + reconciliation adopts strays) rather than fatal, matching the existing non-fatal sidecar-write precedent and keeping the legacy inline test schema working.
One fixture-only adjustment after the red commit: the new-test helper passes DisableDiskCache: maxBytes == 0 to mirror the production mapping; assertions untouched, no pre-existing tests modified.

Also filed #56 for a pre-existing wart found along the way (Cache.Stats reads never-populated tables).

Implementation is up as PR #55 (https://git.eeqj.de/sneak/pixa/pulls/55), labeled `needs-review` and assigned to clawbot for the review loop. It follows the plan comment above; deviations and judgment calls, all detailed in the PR body: 1. **Cache-layer disable encoding**: at the `imgcache.CacheConfig` layer, disabling is an explicit `DisableDiskCache` flag instead of `MaxBytes == 0`, because pre-existing test fixtures construct `CacheConfig` with a zero `MaxBytes` and depend on the legacy no-limit behavior (repo rules forbid touching existing tests). The config layer maps `cache_max_bytes: 0` to the flag; observable behavior matches the direction exactly (verified end-to-end). 2. **Negative cache stays active when the disk cache is disabled** — it is DB-backed and TTL-bounded, not part of the disk cache this issue bounds; flagged in the PR for an explicit call if it should be disabled too. 3. **Variant accounting inserts are best-effort** (warn + reconciliation adopts strays) rather than fatal, matching the existing non-fatal sidecar-write precedent and keeping the legacy inline test schema working. 4. One fixture-only adjustment after the red commit: the new-test helper passes `DisableDiskCache: maxBytes == 0` to mirror the production mapping; assertions untouched, no pre-existing tests modified. Also filed #56 for a pre-existing wart found along the way (`Cache.Stats` reads never-populated tables).

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#51