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

Merged

sneak merged 12 commits from feature/cache-size-eviction into main 2026-08-09 13:22:51 +02:00

Conversation 10 Commits 12 Files Changed 15 +2597 -41

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

Collaborator

Copy Link

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) ... 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

Author

Collaborator

Copy Link

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

clawbot commented 2026-08-09 02:36:18 +02:00

Author

Collaborator

Copy Link

Verdict: FAIL (needs-rework)

Independent review of PR #55 against issue #51 DoD, owner direction (issuecomment-44068), implementer plan (issuecomment-44081), and REPO_POLICIES.md. make check was run green in a local worktree at HEAD (c1ec038), plus go test -race ./... (clean, no races detected) and docker build --target lint . against the pinned golangci-lint v2.10.1 (clean). CI status on c1ec038 is green and the branch is mergeable against current main (61f42e6). None of that is in question — the fail is a policy violation plus unaddressed correctness gaps in the eviction design.

Blocking: iron-rule violation — migration numbering

internal/database/schema/002_cache_eviction.sql is a new file. REPO_POLICIES.md states explicitly:

> Pre-1.0.0: never add additional migration files (002, 003, etc.). There is no installed base to migrate. Edit 001_schema.sql directly.

TODO.md's own Status line confirms pre-1.0. No git tags exist. git log --oneline --diff-filter=A -- 'internal/database/schema/*.sql' shows only 000.sql and 001_initial_schema.sql existed before this PR; commit bdd86a4 is the first to add a 002_*.sql file. The issue's implementer-plan comment (issuecomment-44081) proposed "Migration 002" on its own initiative — that is not an owner override of REPO_POLICIES.md (the review brief's supersession rule applies to issue #51's own DoD/body, not to the separate, written repo policy doc). Per the review brief, a change that violates an iron rule fails regardless of quality.

Fix: fold the variant_content table and source_content.last_accessed_at column directly into 001_initial_schema.sql and drop 002_cache_eviction.sql; there is no installed base to preserve.

Correctness gaps (should fix, not individually fatal, but undisclosed)

1. evictSourceBlob TOCTOU window vs. concurrent content-addressed dedup (internal/imgcache/eviction.go:294-334). references is read once (line 295) before the transaction. The transaction (lines 300-319) deletes source_metadata/source_content rows by a fresh WHERE content_hash = ? query, so a new reference added before the transaction runs is safely swept too. But there is a real gap after tx.Commit() (line 317) and before c.srcContent.Delete(contentHash) (line 329): ContentStorage.Store() (storage.go:56-112) decides "already stored" purely from os.Stat on the content file, which still exists in this window. A concurrent StoreSource call for a different source path whose content happens to hash to the same value (genuine SHA-256 dedup, not a contrived case) will find the file present, skip rewriting it, and INSERT ... ON CONFLICT DO NOTHING into source_content — succeeding as a fresh row, since eviction's row was already deleted — and insert a fresh source_metadata row referencing that hash. Eviction then unlinks the file at line 329, leaving that fresh row pointing at a deleted file: a transient violation of "rows never point at deleted files." It self-heals (LookupSource's srcContent.Exists check at cache.go:324 reports a miss, and the next startup's reconcileSourceRows cleans it up), but it is a real, untested gap in the exact invariant the DoD calls "the trickiest DoD case." None of the eviction tests exercise a write racing the unlink step.

2. One-time-only reconciliation + best-effort accounting insert = unbounded drift risk for long-running processes. evictionLoop (eviction.go:416-440) calls reconcileAccounting exactly once, before entering the ticker/pressure loop — it is never re-run periodically. StoreVariant (cache.go:269-295) makes its variant_content insert best-effort: on failure it only warns (line 288) and still returns success. If that insert fails during the life of a long-running process (e.g. SQLITE_BUSY under concurrent writer contention — internal/database/database.go configures no busy_timeout and no SetMaxOpenConns, so contention between the evictor's deletes and concurrent StoreVariant/StoreSource inserts is not guarded against), the variant file lands on disk untracked and stays untracked — invisible to UsageBytes/EvictToLimit — until the next process restart. That is a live channel for the disk to grow past cache_max_bytes without eviction ever noticing, which is the exact DoS class issue #51 exists to close. This risk is not mentioned in the PR's disclosed-deviations list (which only discloses that the insert is best-effort, not that recovery is restart-gated) and is not covered by any test that simulates a failed accounting insert during steady-state operation.

3. Startup reconciliation races against request serving. StartEviction (eviction.go:391-399) is invoked from the OnStart hook (handlers.go:88) and launches the reconciliation walk in a background goroutine without blocking; nothing prevents the HTTP listener from accepting requests while reconcileSourceFiles/removeUntrackedSourceFile (eviction.go:617-667) are still walking. ContentStorage.Store() renames a new blob into place before its source_content row is inserted (storage.go:56-112 then cache.go:214-221). A source fetch completing in that specific window at cold start can have its just-written blob file treated as "untracked" and deleted by reconciliation before the DB insert lands. Self-healing (the request itself already has the bytes in memory), but it is a silent cache-write loss that isn't exercised by any test and isn't discussed as a known limitation.

4. Minor race, not a safety issue: evictVariant (eviction.go:267-279) can select a variant as an LRU candidate and then delete it after a concurrent StoreVariant has just refreshed that same key (fresh timestamp, fresh bytes) — the freshly-written entry is destroyed rather than kept. No dangling reference results (row and file are deleted together), just wasted work / a reduced hit rate under churn. Noted for completeness, not required to fix.

Process/tooling note (pre-existing, not this PR's fault, but relevant to the concurrency claims)

script/test (unchanged by this PR) runs go test -timeout 30s -v ./... with no -race, so make check/CI never actually exercises the race detector over this PR's new goroutine + channel + shared-DB machinery. I ran CGO_ENABLED=1 go test -timeout 60s -race ./... manually in the PR worktree and it was clean, so I have no evidence of an actual data race, but the review brief's requirement ("confirm make test already does this") does not hold — flagging since a concurrency-heavy PR is exactly where that gap matters most.

What's solid (no changes needed)

• Config: getInt64/cachesize.go strict parsing correctly rejects negative, float, null, bare-key, non-numeric string, boolean, and list values, each with its own test (internal/config/cache_max_bytes_test.go) asserting both key and offending value appear in the error. Explicit values bypass the floor (tested); the 75%/500 MiB floor computation is correct integer arithmetic (divide-then-multiply, overflow-clamped) and both branches (75% dominant, floor dominant, including a "just below threshold" boundary case) are tested via the injectable probe. cache_max_bytes: 0 is proven to create zero cache directories and write zero files (TestZeroMaxBytesDisablesDiskCache).
• Eviction correctness tests are real, not vacuous: TestEvictToLimitEvictsLeastRecentlyUsedFirst backdates four distinct timestamps and asserts the specific victim and specific survivors, not just aggregate byte counts. TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences populates an actual 2-reference blob, forces real eviction via a real byte limit, and asserts both source_metadata rows and both sidecar files are gone together with the row and file, plus assertNoDanglingReferences. Write-pressure and periodic-trigger tests start the real goroutine and poll for the on-disk/DB effect, not just that a function returned without error. Reconciliation adoption/drop is exercised end-to-end (TestStartEvictionReconcilesAccountingWithDisk).
• No linter config changes; 0 lint issues; make check leaves the tree clean (git status empty after); commit hygiene is correct (only the finishing commit c1ec038 carries (closes #51), TDD red/green ordering is real per git show --stat on each commit); no Claude/Anthropic references or attribution trailers anywhere in the log or diff.
• Disclosed judgment calls (DisableDiskCache flag vs. MaxBytes==0, negative cache staying active when disk cache disabled, best-effort variant insert in isolation) are reasonable and adequately flagged for owner sign-off; no objection to any of them individually.

Required for merge-ready

• Fold migration 002 into 001_initial_schema.sql; delete the 002 file (policy violation, blocking).
• At minimum, disclose (ideally fix) items 1-3 above: either make reconciliation periodic (or the variant insert non-best-effort/retried), or explicitly document the accepted drift/race windows as known limitations with a follow-up issue filed (the way #56 was filed for the Cache.Stats finding).

## Verdict: FAIL (needs-rework) Independent review of PR #55 against issue #51 DoD, owner direction (issuecomment-44068), implementer plan (issuecomment-44081), and REPO_POLICIES.md. `make check` was run green in a local worktree at HEAD (c1ec038), plus `go test -race ./...` (clean, no races detected) and `docker build --target lint .` against the pinned golangci-lint v2.10.1 (clean). CI status on c1ec038 is green and the branch is mergeable against current `main` (61f42e6). None of that is in question — the fail is a policy violation plus unaddressed correctness gaps in the eviction design. ### Blocking: iron-rule violation — migration numbering `internal/database/schema/002_cache_eviction.sql` is a new file. REPO_POLICIES.md states explicitly: > Pre-1.0.0: never add additional migration files (002, 003, etc.). There is no installed base to migrate. Edit 001_schema.sql directly. TODO.md's own Status line confirms `pre-1.0. No git tags exist.` `git log --oneline --diff-filter=A -- 'internal/database/schema/*.sql'` shows only `000.sql` and `001_initial_schema.sql` existed before this PR; commit `bdd86a4` is the first to add a `002_*.sql` file. The issue's implementer-plan comment (issuecomment-44081) proposed "Migration 002" on its own initiative — that is not an owner override of REPO_POLICIES.md (the review brief's supersession rule applies to issue #51's own DoD/body, not to the separate, written repo policy doc). Per the review brief, a change that violates an iron rule fails regardless of quality. Fix: fold the `variant_content` table and `source_content.last_accessed_at` column directly into `001_initial_schema.sql` and drop `002_cache_eviction.sql`; there is no installed base to preserve. ### Correctness gaps (should fix, not individually fatal, but undisclosed) 1. **`evictSourceBlob` TOCTOU window vs. concurrent content-addressed dedup** (`internal/imgcache/eviction.go:294-334`). `references` is read once (line 295) before the transaction. The transaction (lines 300-319) deletes `source_metadata`/`source_content` rows by a fresh `WHERE content_hash = ?` query, so a *new* reference added before the transaction runs is safely swept too. But there is a real gap *after* `tx.Commit()` (line 317) and *before* `c.srcContent.Delete(contentHash)` (line 329): `ContentStorage.Store()` (`storage.go:56-112`) decides "already stored" purely from `os.Stat` on the content file, which still exists in this window. A concurrent `StoreSource` call for a different source path whose content happens to hash to the same value (genuine SHA-256 dedup, not a contrived case) will find the file present, skip rewriting it, and `INSERT ... ON CONFLICT DO NOTHING` into `source_content` — succeeding as a fresh row, since eviction's row was already deleted — and insert a fresh `source_metadata` row referencing that hash. Eviction then unlinks the file at line 329, leaving that fresh row pointing at a deleted file: a transient violation of "rows never point at deleted files." It self-heals (LookupSource's `srcContent.Exists` check at cache.go:324 reports a miss, and the next startup's `reconcileSourceRows` cleans it up), but it is a real, untested gap in the exact invariant the DoD calls "the trickiest DoD case." None of the eviction tests exercise a write racing the unlink step. 2. **One-time-only reconciliation + best-effort accounting insert = unbounded drift risk for long-running processes.** `evictionLoop` (`eviction.go:416-440`) calls `reconcileAccounting` exactly once, before entering the ticker/pressure loop — it is never re-run periodically. `StoreVariant` (`cache.go:269-295`) makes its `variant_content` insert best-effort: on failure it only warns (line 288) and still returns success. If that insert fails during the life of a long-running process (e.g. `SQLITE_BUSY` under concurrent writer contention — `internal/database/database.go` configures no `busy_timeout` and no `SetMaxOpenConns`, so contention between the evictor's deletes and concurrent `StoreVariant`/`StoreSource` inserts is not guarded against), the variant file lands on disk untracked and stays untracked — invisible to `UsageBytes`/`EvictToLimit` — until the *next process restart*. That is a live channel for the disk to grow past `cache_max_bytes` without eviction ever noticing, which is the exact DoS class issue #51 exists to close. This risk is not mentioned in the PR's disclosed-deviations list (which only discloses that the insert is best-effort, not that recovery is restart-gated) and is not covered by any test that simulates a failed accounting insert during steady-state operation. 3. **Startup reconciliation races against request serving.** `StartEviction` (`eviction.go:391-399`) is invoked from the `OnStart` hook (`handlers.go:88`) and launches the reconciliation walk in a background goroutine without blocking; nothing prevents the HTTP listener from accepting requests while `reconcileSourceFiles`/`removeUntrackedSourceFile` (`eviction.go:617-667`) are still walking. `ContentStorage.Store()` renames a new blob into place before its `source_content` row is inserted (`storage.go:56-112` then `cache.go:214-221`). A source fetch completing in that specific window at cold start can have its just-written blob file treated as "untracked" and deleted by reconciliation before the DB insert lands. Self-healing (the request itself already has the bytes in memory), but it is a silent cache-write loss that isn't exercised by any test and isn't discussed as a known limitation. 4. **Minor race, not a safety issue:** `evictVariant` (`eviction.go:267-279`) can select a variant as an LRU candidate and then delete it after a concurrent `StoreVariant` has just refreshed that same key (fresh timestamp, fresh bytes) — the freshly-written entry is destroyed rather than kept. No dangling reference results (row and file are deleted together), just wasted work / a reduced hit rate under churn. Noted for completeness, not required to fix. ### Process/tooling note (pre-existing, not this PR's fault, but relevant to the concurrency claims) `script/test` (unchanged by this PR) runs `go test -timeout 30s -v ./...` with no `-race`, so `make check`/CI never actually exercises the race detector over this PR's new goroutine + channel + shared-DB machinery. I ran `CGO_ENABLED=1 go test -timeout 60s -race ./...` manually in the PR worktree and it was clean, so I have no evidence of an actual data race, but the review brief's requirement ("confirm make test already does this") does not hold — flagging since a concurrency-heavy PR is exactly where that gap matters most. ### What's solid (no changes needed) - Config: `getInt64`/`cachesize.go` strict parsing correctly rejects negative, float, null, bare-key, non-numeric string, boolean, and list values, each with its own test (`internal/config/cache_max_bytes_test.go`) asserting both key and offending value appear in the error. Explicit values bypass the floor (tested); the 75%/500 MiB floor computation is correct integer arithmetic (divide-then-multiply, overflow-clamped) and both branches (75% dominant, floor dominant, including a "just below threshold" boundary case) are tested via the injectable probe. `cache_max_bytes: 0` is proven to create zero cache directories and write zero files (`TestZeroMaxBytesDisablesDiskCache`). - Eviction correctness tests are real, not vacuous: `TestEvictToLimitEvictsLeastRecentlyUsedFirst` backdates four distinct timestamps and asserts the specific victim and specific survivors, not just aggregate byte counts. `TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences` populates an actual 2-reference blob, forces real eviction via a real byte limit, and asserts both `source_metadata` rows and both sidecar files are gone together with the row and file, plus `assertNoDanglingReferences`. Write-pressure and periodic-trigger tests start the real goroutine and poll for the on-disk/DB effect, not just that a function returned without error. Reconciliation adoption/drop is exercised end-to-end (`TestStartEvictionReconcilesAccountingWithDisk`). - No linter config changes; 0 lint issues; `make check` leaves the tree clean (`git status` empty after); commit hygiene is correct (only the finishing commit `c1ec038` carries `(closes #51)`, TDD red/green ordering is real per `git show --stat` on each commit); no Claude/Anthropic references or attribution trailers anywhere in the log or diff. - Disclosed judgment calls (`DisableDiskCache` flag vs. `MaxBytes==0`, negative cache staying active when disk cache disabled, best-effort variant insert in isolation) are reasonable and adequately flagged for owner sign-off; no objection to any of them individually. ### Required for merge-ready - Fold migration 002 into `001_initial_schema.sql`; delete the 002 file (policy violation, blocking). - At minimum, disclose (ideally fix) items 1-3 above: either make reconciliation periodic (or the variant insert non-best-effort/retried), or explicitly document the accepted drift/race windows as known limitations with a follow-up issue filed (the way #56 was filed for the `Cache.Stats` finding).

clawbot added needs-rework and removed needs-review labels 2026-08-09 02:36:45 +02:00

clawbot commented 2026-08-09 02:36:48 +02:00

Author

Collaborator

Copy Link

Manager note: independent adversarial review came back FAIL (see the review comment above for full detail). Blocking issue: internal/database/schema/002_cache_eviction.sql violates the pre-1.0 rule in REPO_POLICIES.md that forbids adding numbered migrations beyond 001 before a tagged release — the new table/column must be folded into 001_initial_schema.sql instead. Additional non-blocking-but-real correctness gaps: a TOCTOU window in evictSourceBlob between the row-delete transaction and the file unlink, one-shot (not periodic) reconciliation letting best-effort-failed accounting inserts accumulate untracked disk usage indefinitely on a long-running process, and script/test not running with -race so CI never exercises the race detector over this PR's new concurrency.

Dispatching a rework pass against these findings, then a fresh independent reviewer.

Manager note: independent adversarial review came back FAIL (see the review comment above for full detail). Blocking issue: `internal/database/schema/002_cache_eviction.sql` violates the pre-1.0 rule in REPO_POLICIES.md that forbids adding numbered migrations beyond `001` before a tagged release — the new table/column must be folded into `001_initial_schema.sql` instead. Additional non-blocking-but-real correctness gaps: a TOCTOU window in `evictSourceBlob` between the row-delete transaction and the file unlink, one-shot (not periodic) reconciliation letting best-effort-failed accounting inserts accumulate untracked disk usage indefinitely on a long-running process, and `script/test` not running with `-race` so CI never exercises the race detector over this PR's new concurrency. Dispatching a rework pass against these findings, then a fresh independent reviewer.

clawbot added 8 commits 2026-08-09 02:49:06 +02:00

fix: fold cache eviction schema into 001_initial_schema.sql ... 6b0870d3c9

Pre-1.0 with no installed base to migrate: REPO_POLICIES.md forbids
adding numbered migration files beyond 001 before a tagged release.
Fold the variant_content table and source_content.last_accessed_at
column (previously 002_cache_eviction.sql) directly into
001_initial_schema.sql and delete the 002 file. The migration runner
is generic over whatever *.sql files exist in schema/, so no runner
code changes are needed.

docs: remove stale migration-002 reference from TODO.md ... 314ccbcd9d

Follows the schema fold: the variant_content table and last_accessed_at
column are now part of 001_initial_schema.sql, not a separate migration.

test: add contentLock, a per-key mutex for content-hash exclusion ... ea7621de29

Introduces the keyed exclusion primitive that StoreSource and
evictSourceBlob will hold across their full operation, so a store and
an eviction racing on identical content bytes cannot interleave.
Covered here in isolation: same-key exclusion, independence across
distinct keys, and that the entry map does not grow unbounded.

test: add failing test for evictSourceBlob unlink-vs-store TOCTOU ... 90b2f6fa66

Adds an instrumentation seam (evictSourceBlobTestHook, fired after the
row-deletion transaction commits and before the content file is
unlinked) and a test that pauses eviction there while a concurrent
StoreSource for identical content bytes races it. Currently red: the
store completes immediately instead of being excluded, which is
exactly the window the review flagged between evictSourceBlob's commit
and its unlink.

fix: close TOCTOU window between blob eviction commit and unlink ... 9197b6300a

StoreSource now hashes content itself and holds the per-hash
contentLock across the whole store (file write plus accounting row
inserts); evictSourceBlob holds the same lock across its whole
operation (row deletion transaction through file unlink). A concurrent
store and eviction of identical content bytes can no longer
interleave: either runs to completion before the other starts, so a
fresh row can never be left pointing at a file the other side is
mid-unlink on.

ContentStorage gains StoreHashed for callers that need the hash before
writing; Store is refactored to share the write-if-absent logic with
it, with no change to its existing behavior or signature.

internal/imgcache/eviction_test.go:
TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent proves it:
pauses eviction (via evictSourceBlobTestHook) in the exact window
between commit and unlink, asserts a concurrent StoreSource for
identical content blocks rather than completing, then verifies no
dangling reference and that the store's data survives once eviction
releases the hash.

test: add failing test for one-shot-only reconciliation ... 41347a7e9f

reconcileAccounting currently runs exactly once, at evictor startup.
Combined with StoreVariant's best-effort accounting insert, a variant
file that lands on disk untracked during steady-state operation (e.g.
insert failed under transient DB contention) stays invisible to
UsageBytes/EvictToLimit until the next process restart -- the drift
window the review flagged. Currently red: a file introduced after
startup reconciliation has already run is never adopted.

fix: run accounting reconciliation periodically, not just at startup ... e7964fe777

reconcileAccounting previously ran exactly once, when the evictor
goroutine started. Combined with StoreVariant's best-effort accounting
insert (warns and continues on failure), a long-running process could
accumulate untracked disk usage past cache_max_bytes indefinitely --
the disk-exhaustion failure mode issue #51 exists to close -- with
recovery gated on a process restart.

evictionLoop now also runs a reconciliation pass on every periodic
ticker tick (the same interval eviction itself uses; reconciliation
walks the cache directories so it deliberately does not run on every
write-pressure wakeup, to stay off the per-store hot path). This
bounds unaccounted drift to at most one eviction interval regardless
of how long the process has been running.

TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup proves it:
introduces an untracked variant file only after startup reconciliation
has already completed and asserts a later periodic pass adopts it.

fix: run the test suite with -race ... bdae9cb86b

make check/CI never exercised the race detector over this PR's
concurrency (evictor goroutine, write-pressure channel, the new
per-hash contentLock, concurrent store/evict). Minimal, scoped change:
add -race to the existing go test invocation. Full suite is clean
under it (make check passes; go test -race ./... completes in under
7s, well inside the 30s timeout).

clawbot commented 2026-08-09 02:49:32 +02:00

Author

Collaborator

Copy Link

Rework against the FAIL review (issuecomment-45064) and the manager note (issuecomment-45069). New commits on feature/cache-size-eviction, head bdae9cb (previous head c1ec038):

1. Migration-numbering policy violation (blocking) — fixed.
internal/database/schema/002_cache_eviction.sql is deleted; the variant_content table and source_content.last_accessed_at column (with their indexes) are folded directly into 001_initial_schema.sql (commit 6b0870d). No installed base existed to preserve. The migration runner (internal/database/database.go) is generic over whatever *.sql files exist in schema/, so it needed no changes; TestApplyMigrations_CreatesSchemaAndTables (asserts >= 2 migrations, versions 0 and 1) still passes unmodified. Also removed the now-stale "migration 002" wording from TODO.md (314ccbc).

2. TOCTOU window in evictSourceBlob (commit + unlink vs. concurrent dedup store) — fixed.
Added contentLock, a per-key mutex (internal/imgcache/contentlock.go, tests in contentlock_test.go: same-key exclusion, independence across distinct keys, and that the entry map doesn't grow unbounded). StoreSource now hashes content itself up front and holds that hash's lock across the whole store (content write + accounting inserts); evictSourceBlob holds the same lock across its whole operation (row-deletion transaction through file unlink). The two can no longer interleave: either fully completes before the other starts. ContentStorage gained StoreHashed for the pre-hashed path; Store was refactored to share the write-if-absent logic with it, no change to its existing signature or behavior (internal/imgcache/storage.go).

Evidence: TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent (internal/imgcache/eviction_test.go) pauses eviction — via a test-only hook fired after the delete transaction commits and before the unlink — at exactly the window the review flagged, then races a concurrent StoreSource for a different path with identical content bytes against it. Confirmed red against the pre-fix code (err=<nil>, store completed instead of blocking; commit 90b2f6f), green after the fix (9197b63): the store blocks for the full pause, then completes once eviction releases the hash, assertNoDanglingReferences passes, and the re-stored blob is confirmed present (row + file both exist).

3. One-shot reconciliation + best-effort insert = unbounded drift risk — fixed.
evictionLoop now runs a reconciliation pass on every periodic ticker tick, not just once at startup (internal/imgcache/eviction.go, commit e7964fe). Reconciliation walks the cache directories, so it deliberately only runs on the ticker (not on every write-pressure wakeup) to stay off the per-store hot path; it reuses the same interval eviction itself uses, documented inline — the simplest choice that still bounds unaccounted drift to one eviction interval regardless of process uptime.

Evidence: TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup starts the evictor, lets startup reconciliation settle on an empty cache, then writes a variant file directly to disk (bypassing StoreVariant, i.e. simulating its accounting insert having failed while the file write succeeded) and asserts a later pass adopts it. Confirmed red against one-shot reconciliation (41347a7: usage stayed 0, adoption never happened), green after the fix (e7964fe).

4. Race detector coverage — fixed.
script/test now runs CGO_ENABLED=1 go test -timeout 30s -race -v ./... (commit bdae9cb), scoped to exactly that flag addition as directed — no rewrite of the conditional-verbose-rerun pattern. go test -race ./... across the full module completes in ~6-7s, well inside the 30s timeout, and is clean: no races reported, including around the new contentLock, the evictor goroutine, the write-pressure channel, and the concurrency test above.

Verification: make check (test, lint, fmt-check) is green at bdae9cb; go test -race ./... clean; git status clean after. All new tests follow red-then-green per repo TDD rules (each fix's test committed failing before its implementation commit). No existing test assertions were modified.

Label/assignee left as-is for the next independent review pass.

Rework against the FAIL review (issuecomment-45064) and the manager note (issuecomment-45069). New commits on `feature/cache-size-eviction`, head `bdae9cb` (previous head `c1ec038`): **1. Migration-numbering policy violation (blocking) — fixed.** `internal/database/schema/002_cache_eviction.sql` is deleted; the `variant_content` table and `source_content.last_accessed_at` column (with their indexes) are folded directly into `001_initial_schema.sql` (commit `6b0870d`). No installed base existed to preserve. The migration runner (`internal/database/database.go`) is generic over whatever `*.sql` files exist in `schema/`, so it needed no changes; `TestApplyMigrations_CreatesSchemaAndTables` (asserts >= 2 migrations, versions 0 and 1) still passes unmodified. Also removed the now-stale "migration 002" wording from `TODO.md` (`314ccbc`). **2. TOCTOU window in `evictSourceBlob` (commit + unlink vs. concurrent dedup store) — fixed.** Added `contentLock`, a per-key mutex (`internal/imgcache/contentlock.go`, tests in `contentlock_test.go`: same-key exclusion, independence across distinct keys, and that the entry map doesn't grow unbounded). `StoreSource` now hashes content itself up front and holds that hash's lock across the whole store (content write + accounting inserts); `evictSourceBlob` holds the same lock across its whole operation (row-deletion transaction through file unlink). The two can no longer interleave: either fully completes before the other starts. `ContentStorage` gained `StoreHashed` for the pre-hashed path; `Store` was refactored to share the write-if-absent logic with it, no change to its existing signature or behavior (`internal/imgcache/storage.go`). Evidence: `TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent` (`internal/imgcache/eviction_test.go`) pauses eviction — via a test-only hook fired after the delete transaction commits and before the unlink — at exactly the window the review flagged, then races a concurrent `StoreSource` for a different path with identical content bytes against it. Confirmed red against the pre-fix code (`err=<nil>`, store completed instead of blocking; commit `90b2f6f`), green after the fix (`9197b63`): the store blocks for the full pause, then completes once eviction releases the hash, `assertNoDanglingReferences` passes, and the re-stored blob is confirmed present (row + file both exist). **3. One-shot reconciliation + best-effort insert = unbounded drift risk — fixed.** `evictionLoop` now runs a reconciliation pass on every periodic ticker tick, not just once at startup (`internal/imgcache/eviction.go`, commit `e7964fe`). Reconciliation walks the cache directories, so it deliberately only runs on the ticker (not on every write-pressure wakeup) to stay off the per-store hot path; it reuses the same interval eviction itself uses, documented inline — the simplest choice that still bounds unaccounted drift to one eviction interval regardless of process uptime. Evidence: `TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup` starts the evictor, lets startup reconciliation settle on an empty cache, then writes a variant file directly to disk (bypassing `StoreVariant`, i.e. simulating its accounting insert having failed while the file write succeeded) and asserts a later pass adopts it. Confirmed red against one-shot reconciliation (`41347a7`: usage stayed 0, adoption never happened), green after the fix (`e7964fe`). **4. Race detector coverage — fixed.** `script/test` now runs `CGO_ENABLED=1 go test -timeout 30s -race -v ./...` (commit `bdae9cb`), scoped to exactly that flag addition as directed — no rewrite of the conditional-verbose-rerun pattern. `go test -race ./...` across the full module completes in ~6-7s, well inside the 30s timeout, and is clean: no races reported, including around the new `contentLock`, the evictor goroutine, the write-pressure channel, and the concurrency test above. **Verification:** `make check` (test, lint, fmt-check) is green at `bdae9cb`; `go test -race ./...` clean; `git status` clean after. All new tests follow red-then-green per repo TDD rules (each fix's test committed failing before its implementation commit). No existing test assertions were modified. Label/assignee left as-is for the next independent review pass.

clawbot added needs-review and removed needs-rework labels 2026-08-09 02:49:49 +02:00

clawbot commented 2026-08-09 02:49:51 +02:00

Author

Collaborator

Copy Link

Manager note: rework pass complete (HEAD bdae9cb), see the comment above for the per-finding evidence. Migration 002 folded back into 001_initial_schema.sql, a per-content-hash lock now serializes StoreSource against evictSourceBlob to close the TOCTOU window (new regression test), reconciliation now also runs on every periodic tick rather than once at startup (new regression test), and script/test now runs with -race. make check green and go test -race ./... clean.

Labeling back to needs-review for a fresh independent reviewer.

Manager note: rework pass complete (HEAD `bdae9cb`), see the comment above for the per-finding evidence. Migration `002` folded back into `001_initial_schema.sql`, a per-content-hash lock now serializes `StoreSource` against `evictSourceBlob` to close the TOCTOU window (new regression test), reconciliation now also runs on every periodic tick rather than once at startup (new regression test), and `script/test` now runs with `-race`. `make check` green and `go test -race ./...` clean. Labeling back to `needs-review` for a fresh independent reviewer.

clawbot commented 2026-08-09 02:55:05 +02:00

Author

Collaborator

Copy Link

Verdict: PASS

Independent re-review of PR #55 at head bdae9cb (second round, after the FAIL at c1ec038 in comment #45064). Checked out bdae9cb in a clean worktree; verified all four rework claims independently against the actual code and by running the tests myself, and re-verified the original review scope rather than rubber-stamping it.

1. Migration numbering — fixed, verified

git log --all --oneline -- 'internal/database/schema/*.sql' on this branch shows only 000.sql and 001_initial_schema.sql ever existed after commit 6b0870d ("fix: fold cache eviction schema into 001_initial_schema.sql"); no 002_*.sql file exists in the tree or in branch history after that commit. internal/database/schema/001_initial_schema.sql contains the variant_content table (lines 48-56) and source_content.last_accessed_at (line 13) plus both new indexes, folded directly into the file. grep -rn "002" internal/database/ TODO.md README.md config.example.yml turns up nothing but an unrelated illustrative comment in database.go:46 ("e.g. "001", "002"") describing the general filename-parsing scheme, not a real migration reference. TestApplyMigrations_CreatesSchemaAndTables still passes. TODO.md's stale "migration 002" wording was also removed (commit 314ccbc). Matches REPO_POLICIES.md's pre-1.0 rule exactly.

2. TOCTOU fix (contentlock.go) — fixed, verified

Read internal/imgcache/contentlock.go in full: a reference-counted per-key mutex (entries map[string]*contentLockEntry, count protected by an outer sync.Mutex, entry removed from the map only when its holder/waiter count reaches zero). This is a correct, standard keyed-mutex implementation — not global (proven by TestContentLockAllowsDifferentKeys, which requires all 20 goroutines on distinct keys to reach a rendezvous point simultaneously, timing out after 2s if they were serialized on one lock), doesn't leak entries (TestContentLockRemovesEntryAfterUnlock), and correctly excludes same-key holders (TestContentLockExcludesSameKey, asserts max concurrent holders == 1 across 20 goroutines).

Call sites: StoreSource (cache.go:210-304) hashes content itself before acquiring the lock (so the hash is known up front), then does unlock := c.contentLocks.Lock(string(contentHash)); defer unlock() at line 243-244 covering content write, both DB inserts, and the metadata sidecar write — released via defer on every return path including errors. evictSourceBlob (eviction.go:302-349) does the same at line 303-304, covering the references query, the full delete transaction (commit at line 328), and the file/sidecar unlinks (through line 346) — again via defer, so no leak on any error return. The lock genuinely spans transaction-commit-through-unlink, closing exactly the window the first review flagged.

Regression test TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent (eviction_test.go:744-838) uses a test-only hook fired after the delete transaction commits and before the unlink, pauses eviction there, and races a concurrent StoreSource of identical content against it. It asserts (with a 200ms timeout) that the concurrent store does not complete while eviction holds the hash — this would fail immediately against a reverted/no-op lock, since the store would have nothing blocking it and would return well within 200ms. This is a real, race-forcing test, not a rubber-stamp assertion. Ran CGO_ENABLED=1 go test -timeout 30s -race ./... myself: clean, no races, all packages pass (internal/imgcache in 3.86s).

3. Periodic reconciliation fix — fixed, verified

evictionLoop (eviction.go:432-463) now calls c.runReconciliationPass(ctx) both once before entering the loop (startup) and again inside the select's case <-ticker.C branch (line 447-457) on every subsequent tick — confirmed by reading the code directly, not taking the PR body's word for it. TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup (eviction_test.go:680-734) starts the evictor with a 100ms interval, sleeps 3 intervals to let startup reconciliation settle on an empty cache, then writes an untracked variant file directly to disk (bypassing StoreVariant), and polls for it to be adopted (UsageBytes reaching 900, one accounting row appearing) within a 5s deadline. Because the file is introduced strictly after startup reconciliation already ran and settled, this genuinely exercises the periodic path, not a relabeled startup test.

On the design question raised: reconciliation is deliberately kept off the write-pressure trigger (only runs on the ticker, not on every store notification), staying off the per-store hot path, and is documented inline as such (eviction.go:448-456). It does still do a real filepath.WalkDir plus a stat-per-tracked-row pass (reconcileVariantRows) on every 5-minute tick now rather than once — a legitimate, disclosed-in-comments tradeoff (bounds accounting drift to one interval regardless of uptime) rather than a defect, but worth naming as a residual, non-blocking consideration: for very large caches this adds recurring directory-walk and per-row stat I/O to a background goroutine every interval, and because evictionLoop's select only observes evictionStop between passes (not during one), a reconciliation pass in flight when OnStop fires will make graceful shutdown wait for that pass to finish — StopEviction blocks on <-c.evictionDone with no timeout. Not a correctness bug and not something the DoD requires fixing, but flagging for awareness since it's a new-to-this-PR periodic cost, not a one-time startup cost.

4. Race detector coverage — fixed, verified

git diff 61f42e6..bdae9cb -- script/test shows exactly the intended one-line change: go test -timeout 30s -v ./... → go test -timeout 30s -race -v ./.... Ran it myself (CGO_ENABLED=1 go test -timeout 30s -race ./..., full module): clean in ~7s, all packages pass, no races reported, including internal/imgcache (which carries all the new lock/goroutine/channel code) and internal/config. make check also green end-to-end (~26s wall, includes lint + fmt-check).

Re-verification of original scope (not just re-trusting the first PASS items)

• getInt64/cachesize.go (internal/config/config.go:539-587, cachesize.go): strict parsing rejects negative (via validate(), config.go:319-323), non-integer float, null, non-numeric string, and any other type (bool, list) falls through to the default: case and errors — confirmed by reading the switch directly, consistent in structure and idiom with the existing getInt/getBool strict getters. cache_max_bytes: 0 is accepted (only < 0 is rejected) and disables the cache end-to-end (TestZeroMaxBytesDisablesDiskCache asserts zero DB rows, zero files, no cache/ directory created at all — real filesystem walk, not just a flag check).
• 75%/500 MiB default: ComputeDefaultCacheMaxBytes (cachesize.go:55-74) divides before multiplying (overflow-safe) and clamps to math.MaxInt64; floor applies only to the computed default (resolveCacheMaxBytes, only entered when !cacheMaxBytesExplicit), never to explicit values (TestCacheMaxBytesExplicitValueUsedWithoutFloor, TestResolveCacheMaxBytesDoesNotOverrideExplicitValue).
• Multi-reference blob eviction (TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences, eviction_test.go:340-415): real 2-reference blob, real eviction via a real byte limit, asserts the blob is gone from disk, from source_content, and from both source_metadata rows, both JSON sidecars removed, the untouched (more-recently-used) blob and its single reference survive intact, plus assertNoDanglingReferences. Not vacuous.
• make check (test + lint + fmt-check via the Makefile/script/ entrypoints only) is green at bdae9cb; local golangci-lint version is 2.10.1, matching the Dockerfile-pinned lint image exactly (golangci/golangci-lint:v2.10.1-alpine), so the 0-issues result is meaningful against the same version CI uses. .golangci.yml untouched. git status clean after make check (no files modified).
• CI on bdae9cb: check / check status is success. PR reports mergeable: true; origin/main is still at 61f42e6, the PR's own base, so there is no drift to reconcile — trivially mergeable.
• Commit hygiene: no Claude/Anthropic references or attribution trailers anywhere in the branch's commit messages or diff (checked explicitly). (closes #51) is present on c1ec038, the original finishing commit that remains part of this PR's history; the repo's default merge style is squash, so the eventual merge commit message will derive from the PR title, which also ends (closes #51). TDD ordering for all three rework fixes is real red-then-green (test: add failing test for X commits precede their fix: commits, each independently confirmed failing/passing per the rework report and consistent with what's in the diff).
• Docs: README.md and config.example.yml both document cache_max_bytes accurately; TODO.md Workflow bookkeeping is correct (P1 blocked-networks promoted to Next Step, Completed Steps entry added, stale migration wording removed), and formatted per repo conventions.
• No inclusive-terminology issues, no linter config changes, no scope creep — every changed file (config, schema, imgcache eviction/storage/lock, handlers wiring, docs, script/test) is directly attributable to this issue's DoD.

Non-blocking observations (not required for merge)

• Periodic reconciliation's directory walk + per-row stat pass now recurs every 5 minutes rather than running once; this is a reasonable, disclosed tradeoff but is a new recurring background I/O cost worth watching at scale, and can add latency to StopEviction/graceful shutdown if a pass is in flight when OnStop fires (no timeout on the wait). Worth a follow-up issue if it ever shows up in practice, not a blocker here.
• The still-open, previously-noted minor race (evictVariant can destroy a variant a concurrent StoreVariant just refreshed) remains unaddressed, but was explicitly called out by the previous review as non-blocking ("just wasted work / reduced hit rate under churn, no dangling reference"), and that assessment still holds — not part of the required fix set.

No blocking findings. This PR may be labeled merge-ready and assigned to sneak.

## Verdict: PASS Independent re-review of PR #55 at head `bdae9cb` (second round, after the FAIL at `c1ec038` in comment #45064). Checked out `bdae9cb` in a clean worktree; verified all four rework claims independently against the actual code and by running the tests myself, and re-verified the original review scope rather than rubber-stamping it. ### 1. Migration numbering — fixed, verified `git log --all --oneline -- 'internal/database/schema/*.sql'` on this branch shows only `000.sql` and `001_initial_schema.sql` ever existed after commit `6b0870d` ("fix: fold cache eviction schema into 001_initial_schema.sql"); no `002_*.sql` file exists in the tree or in branch history after that commit. `internal/database/schema/001_initial_schema.sql` contains the `variant_content` table (lines 48-56) and `source_content.last_accessed_at` (line 13) plus both new indexes, folded directly into the file. `grep -rn "002" internal/database/ TODO.md README.md config.example.yml` turns up nothing but an unrelated illustrative comment in `database.go:46` ("e.g. \"001\", \"002\"") describing the general filename-parsing scheme, not a real migration reference. `TestApplyMigrations_CreatesSchemaAndTables` still passes. `TODO.md`'s stale "migration 002" wording was also removed (commit `314ccbc`). Matches REPO_POLICIES.md's pre-1.0 rule exactly. ### 2. TOCTOU fix (`contentlock.go`) — fixed, verified Read `internal/imgcache/contentlock.go` in full: a reference-counted per-key mutex (`entries map[string]*contentLockEntry`, count protected by an outer `sync.Mutex`, entry removed from the map only when its holder/waiter count reaches zero). This is a correct, standard keyed-mutex implementation — not global (proven by `TestContentLockAllowsDifferentKeys`, which requires all 20 goroutines on distinct keys to reach a rendezvous point simultaneously, timing out after 2s if they were serialized on one lock), doesn't leak entries (`TestContentLockRemovesEntryAfterUnlock`), and correctly excludes same-key holders (`TestContentLockExcludesSameKey`, asserts max concurrent holders == 1 across 20 goroutines). Call sites: `StoreSource` (`cache.go:210-304`) hashes content itself before acquiring the lock (so the hash is known up front), then does `unlock := c.contentLocks.Lock(string(contentHash)); defer unlock()` at line 243-244 covering content write, both DB inserts, and the metadata sidecar write — released via `defer` on every return path including errors. `evictSourceBlob` (`eviction.go:302-349`) does the same at line 303-304, covering the references query, the full delete transaction (commit at line 328), and the file/sidecar unlinks (through line 346) — again via `defer`, so no leak on any error return. The lock genuinely spans transaction-commit-through-unlink, closing exactly the window the first review flagged. Regression test `TestEvictSourceBlobExcludesConcurrentStoreOfIdenticalContent` (`eviction_test.go:744-838`) uses a test-only hook fired after the delete transaction commits and before the unlink, pauses eviction there, and races a concurrent `StoreSource` of identical content against it. It asserts (with a 200ms timeout) that the concurrent store does *not* complete while eviction holds the hash — this would fail immediately against a reverted/no-op lock, since the store would have nothing blocking it and would return well within 200ms. This is a real, race-forcing test, not a rubber-stamp assertion. Ran `CGO_ENABLED=1 go test -timeout 30s -race ./...` myself: clean, no races, all packages pass (`internal/imgcache` in 3.86s). ### 3. Periodic reconciliation fix — fixed, verified `evictionLoop` (`eviction.go:432-463`) now calls `c.runReconciliationPass(ctx)` both once before entering the loop (startup) and again inside the `select`'s `case <-ticker.C` branch (line 447-457) on every subsequent tick — confirmed by reading the code directly, not taking the PR body's word for it. `TestPeriodicReconciliationAdoptsFileThatAppearsAfterStartup` (`eviction_test.go:680-734`) starts the evictor with a 100ms interval, sleeps 3 intervals to let startup reconciliation settle on an empty cache, *then* writes an untracked variant file directly to disk (bypassing `StoreVariant`), and polls for it to be adopted (`UsageBytes` reaching 900, one accounting row appearing) within a 5s deadline. Because the file is introduced strictly after startup reconciliation already ran and settled, this genuinely exercises the periodic path, not a relabeled startup test. On the design question raised: reconciliation is deliberately kept off the write-pressure trigger (only runs on the ticker, not on every store notification), staying off the per-store hot path, and is documented inline as such (`eviction.go:448-456`). It does still do a real `filepath.WalkDir` plus a stat-per-tracked-row pass (`reconcileVariantRows`) on every 5-minute tick now rather than once — a legitimate, disclosed-in-comments tradeoff (bounds accounting drift to one interval regardless of uptime) rather than a defect, but worth naming as a residual, non-blocking consideration: for very large caches this adds recurring directory-walk and per-row stat I/O to a background goroutine every interval, and because `evictionLoop`'s `select` only observes `evictionStop` between passes (not during one), a reconciliation pass in flight when `OnStop` fires will make graceful shutdown wait for that pass to finish — `StopEviction` blocks on `<-c.evictionDone` with no timeout. Not a correctness bug and not something the DoD requires fixing, but flagging for awareness since it's a new-to-this-PR periodic cost, not a one-time startup cost. ### 4. Race detector coverage — fixed, verified `git diff 61f42e6..bdae9cb -- script/test` shows exactly the intended one-line change: `go test -timeout 30s -v ./...` → `go test -timeout 30s -race -v ./...`. Ran it myself (`CGO_ENABLED=1 go test -timeout 30s -race ./...`, full module): clean in ~7s, all packages pass, no races reported, including `internal/imgcache` (which carries all the new lock/goroutine/channel code) and `internal/config`. `make check` also green end-to-end (~26s wall, includes lint + fmt-check). ### Re-verification of original scope (not just re-trusting the first PASS items) - `getInt64`/`cachesize.go` (`internal/config/config.go:539-587`, `cachesize.go`): strict parsing rejects negative (via `validate()`, `config.go:319-323`), non-integer float, null, non-numeric string, and any other type (bool, list) falls through to the `default:` case and errors — confirmed by reading the switch directly, consistent in structure and idiom with the existing `getInt`/`getBool` strict getters. `cache_max_bytes: 0` is accepted (only `< 0` is rejected) and disables the cache end-to-end (`TestZeroMaxBytesDisablesDiskCache` asserts zero DB rows, zero files, no `cache/` directory created at all — real filesystem walk, not just a flag check). - 75%/500 MiB default: `ComputeDefaultCacheMaxBytes` (`cachesize.go:55-74`) divides before multiplying (overflow-safe) and clamps to `math.MaxInt64`; floor applies only to the computed default (`resolveCacheMaxBytes`, only entered when `!cacheMaxBytesExplicit`), never to explicit values (`TestCacheMaxBytesExplicitValueUsedWithoutFloor`, `TestResolveCacheMaxBytesDoesNotOverrideExplicitValue`). - Multi-reference blob eviction (`TestEvictionRemovesMultiReferencedBlobTogetherWithAllReferences`, `eviction_test.go:340-415`): real 2-reference blob, real eviction via a real byte limit, asserts the blob is gone from disk, from `source_content`, and from *both* `source_metadata` rows, both JSON sidecars removed, the untouched (more-recently-used) blob and its single reference survive intact, plus `assertNoDanglingReferences`. Not vacuous. - `make check` (test + lint + fmt-check via the Makefile/`script/` entrypoints only) is green at `bdae9cb`; local `golangci-lint version` is `2.10.1`, matching the Dockerfile-pinned lint image exactly (`golangci/golangci-lint:v2.10.1-alpine`), so the 0-issues result is meaningful against the same version CI uses. `.golangci.yml` untouched. `git status` clean after `make check` (no files modified). - CI on `bdae9cb`: `check / check` status is `success`. PR reports `mergeable: true`; `origin/main` is still at `61f42e6`, the PR's own base, so there is no drift to reconcile — trivially mergeable. - Commit hygiene: no Claude/Anthropic references or attribution trailers anywhere in the branch's commit messages or diff (checked explicitly). `(closes #51)` is present on `c1ec038`, the original finishing commit that remains part of this PR's history; the repo's default merge style is squash, so the eventual merge commit message will derive from the PR title, which also ends `(closes #51)`. TDD ordering for all three rework fixes is real red-then-green (`test: add failing test for X` commits precede their `fix:` commits, each independently confirmed failing/passing per the rework report and consistent with what's in the diff). - Docs: `README.md` and `config.example.yml` both document `cache_max_bytes` accurately; `TODO.md` Workflow bookkeeping is correct (P1 blocked-networks promoted to Next Step, Completed Steps entry added, stale migration wording removed), and formatted per repo conventions. - No inclusive-terminology issues, no linter config changes, no scope creep — every changed file (config, schema, imgcache eviction/storage/lock, handlers wiring, docs, `script/test`) is directly attributable to this issue's DoD. ### Non-blocking observations (not required for merge) - Periodic reconciliation's directory walk + per-row stat pass now recurs every 5 minutes rather than running once; this is a reasonable, disclosed tradeoff but is a new recurring background I/O cost worth watching at scale, and can add latency to `StopEviction`/graceful shutdown if a pass is in flight when `OnStop` fires (no timeout on the wait). Worth a follow-up issue if it ever shows up in practice, not a blocker here. - The still-open, previously-noted minor race (`evictVariant` can destroy a variant a concurrent `StoreVariant` just refreshed) remains unaddressed, but was explicitly called out by the previous review as non-blocking ("just wasted work / reduced hit rate under churn, no dangling reference"), and that assessment still holds — not part of the required fix set. No blocking findings. This PR may be labeled `merge-ready` and assigned to sneak.

clawbot added merge-ready and removed needs-review labels 2026-08-09 02:55:22 +02:00

clawbot removed their assignment 2026-08-09 02:55:23 +02:00

sneak was assigned by clawbot 2026-08-09 02:55:23 +02:00

clawbot commented 2026-08-09 02:55:25 +02:00

Author

Collaborator

Copy Link

Manager note: independent second-round review PASS (see comment above) — all four round-1 findings independently re-verified as genuinely fixed (migration folded back into 001_initial_schema.sql, TOCTOU closed via a real per-content-hash lock with a race that was reasoned to actually catch the original bug, reconciliation now runs every periodic tick, -race enabled in script/test and clean). Original scope re-checked and still holds; CI green on bdae9cb.

Labeling merge-ready and assigning sneak to merge (protected main).

Manager note: independent second-round review PASS (see comment above) — all four round-1 findings independently re-verified as genuinely fixed (migration folded back into `001_initial_schema.sql`, TOCTOU closed via a real per-content-hash lock with a race that was reasoned to actually catch the original bug, reconciliation now runs every periodic tick, `-race` enabled in `script/test` and clean). Original scope re-checked and still holds; CI green on `bdae9cb`. Labeling `merge-ready` and assigning `sneak` to merge (protected `main`).

clawbot referenced this pull request 2026-08-09 03:00:22 +02:00

script/test doesn't follow the REPO_POLICIES conditional-verbose-rerun pattern (always runs -v) #59

clawbot referenced this pull request 2026-08-09 03:31:42 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot commented 2026-08-09 03:32:03 +02:00

Author

Collaborator

Copy Link

Manager note: this PR stays merge-ready and assigned to sneak — no action needed here — but it should be merged BEFORE #54.

Re-verified independently just now: feature/cache-size-eviction at bdae9cb still merges cleanly onto current main (61f42e6, unchanged), and CI check / check (push) is success on that head. Nothing about this PR has regressed.

The reason for the ordering note is a collision between this PR and #54 that neither PR's isolated review could have caught. Both branch from 61f42e6, both merge cleanly onto main individually, but they conflict with each other. Verified by actually performing the merge in a temp worktree:

CONFLICT (content): Merge conflict in TODO.md
CONFLICT (content): Merge conflict in internal/config/config.go
CONFLICT (content): Merge conflict in internal/imgcache/cache.go
CONFLICT (content): Merge conflict in internal/imgcache/storage.go

Additionally, this branch still carries the pre-canonical .golangci.yml (sha256 7b38c4ef3c8cf1f3be006f0f8c980169c9f26a6361bfada32efeb00d8056eb9d, same as main) and the old golangci/golangci-lint:v2.10.1-alpine Dockerfile pin, so the roughly 2,600 lines added here — internal/imgcache/eviction.go, internal/imgcache/contentlock.go, internal/config/cachesize.go and their tests — have never been linted under the canonical v2.12.2 config that #54 introduces.

I am deliberately not asking this PR to absorb that work. Bringing newly-landed code into canonical-config conformance is #54's entire purpose, and #54 has already done exactly that once for #53's config-validation code after it landed. Forcing a large mechanical lint refactor onto this PR instead would reopen review surface on a correctness-sensitive concurrency change that has already passed adversarial review — the wrong risk trade.

So: merge this first. #54 has been moved to needs-rebase and reassigned to clawbot; once this lands, a rework pass will rebase #54 onto the new main, run the canonical-config lint pass over the eviction code, and go back through a fresh independent review.

One consequence worth stating plainly for the record: the lint conformance findings on this PR's new code are being deferred to #54 rather than waived. They will be fixed there, under an independent review, before #54 merges.

Manager note: **this PR stays `merge-ready` and assigned to `sneak` — no action needed here — but it should be merged BEFORE #54.** Re-verified independently just now: `feature/cache-size-eviction` at `bdae9cb` still merges cleanly onto current `main` (`61f42e6`, unchanged), and CI `check / check (push)` is `success` on that head. Nothing about this PR has regressed. The reason for the ordering note is a collision between this PR and #54 that neither PR's isolated review could have caught. Both branch from `61f42e6`, both merge cleanly onto `main` individually, but they conflict with each other. Verified by actually performing the merge in a temp worktree: ``` CONFLICT (content): Merge conflict in TODO.md CONFLICT (content): Merge conflict in internal/config/config.go CONFLICT (content): Merge conflict in internal/imgcache/cache.go CONFLICT (content): Merge conflict in internal/imgcache/storage.go ``` Additionally, this branch still carries the pre-canonical `.golangci.yml` (sha256 `7b38c4ef3c8cf1f3be006f0f8c980169c9f26a6361bfada32efeb00d8056eb9d`, same as `main`) and the old `golangci/golangci-lint:v2.10.1-alpine` Dockerfile pin, so the roughly 2,600 lines added here — `internal/imgcache/eviction.go`, `internal/imgcache/contentlock.go`, `internal/config/cachesize.go` and their tests — have never been linted under the canonical v2.12.2 config that #54 introduces. I am deliberately **not** asking this PR to absorb that work. Bringing newly-landed code into canonical-config conformance is #54's entire purpose, and #54 has already done exactly that once for #53's config-validation code after it landed. Forcing a large mechanical lint refactor onto this PR instead would reopen review surface on a correctness-sensitive concurrency change that has already passed adversarial review — the wrong risk trade. So: **merge this first.** #54 has been moved to `needs-rebase` and reassigned to `clawbot`; once this lands, a rework pass will rebase #54 onto the new `main`, run the canonical-config lint pass over the eviction code, and go back through a fresh independent review. One consequence worth stating plainly for the record: the lint conformance findings on this PR's new code are being deferred to #54 rather than waived. They will be fixed there, under an independent review, before #54 merges.

sneak commented 2026-08-09 03:33:30 +02:00

Owner

Copy Link

what is the periodic reconciliation for, and what is the adoption thing? do we need this?

what is the periodic reconciliation for, and what is the adoption thing? do we need this?

clawbot commented 2026-08-09 03:36:19 +02:00

Author

Collaborator

Copy Link

Answering @sneak's question (issuecomment-45166). Read the code on the branch to answer this rather than quoting the PR body.

What reconciliation is for

Size accounting in this PR is a derived view: UsageBytes is two SUM queries over variant_content and source_content. Eviction only ever sees bytes that have a row. So the whole scheme depends on "every file on disk has an accounting row, and every row has a file" — and there is no way to make a filesystem write and a SQLite insert atomic with each other. Reconciliation is the repair pass for the gap. Concretely it fixes four kinds of drift:

• File on disk, no row — the process died between writing the file and inserting the row, or the insert failed. Those bytes are invisible to eviction forever. This is the dangerous direction: it is silent, unbounded disk growth, i.e. exactly the DoS that #51 exists to close.
• Row, no file — died between deleting the row and unlinking, or someone deleted files underneath us. Over-counts usage, so the evictor evicts real data it did not need to.
• Source blob the DB has no knowledge of — unreachable content taking space.
• .tmp-* leftovers from crashed writes.

What "adoption" is

adoptVariantFile (eviction.go) is the fix for case 1, for variants specifically. It walks the variant directory, and for a file with no variant_content row it inserts one, taking size_bytes from stat, created_at/last_accessed_at from the file mtime, and content_type from the .meta sidecar (falling back to application/octet-stream). "Adopt" = start counting this orphan file as cache usage, which also makes it eligible for eviction. Before adoption it is a file nothing will ever delete and nothing counts.

It matters for variants and not sources because the two store paths differ: StoreSource treats its source_content insert as fatal (returns the error, store fails), while StoreVariant's insert is deliberately best-effort — on failure it logs a warning and still returns success, leaving a file with no row.

Do we need it?

Three separable pieces, and the honest answer differs for each:

1. Startup reconciliation — yes. Crash recovery is real and there is no alternative; without it every unclean shutdown permanently leaks untracked bytes, and they accumulate across restarts.
2. Adoption — yes, as long as any path can write a file without a row. Today two paths can: crashes (unavoidable) and StoreVariant's best-effort insert (avoidable).
3. Re-running it on every 5-minute tick — this exists only because of StoreVariant's best-effort insert. That was round 1's finding: with startup-only reconciliation, an insert that fails at hour 3 of a 30-day process stays unaccounted for 30 days. Making the pass periodic bounds that to one interval. It is a patch over the best-effort insert, not an independently motivated feature.

And I want to flag a cost the reviewer only partly named. adoptVariantFile issues one SELECT COUNT(*) FROM variant_content WHERE cache_key = ? per file, and reconcileVariantRows stats every tracked row — on every tick, now, not once. On a cache sized at the default (75% of free space, easily hundreds of GB, millions of variants) that is millions of SQLite queries plus a full WalkDir every 5 minutes, against a proxy whose README targets 1k-5k req/s. It is off the request path and shutdown-safe-ish, but StopEviction waits on an in-flight pass with no timeout, so it can also drag out graceful shutdown. At small cache sizes this is free; at the sizes the default config picks it is not.

Options

A. Merge #55 as-is, file the simplification as a follow-up. Current behavior is correct — two adversarial reviews confirmed it — just wasteful at scale. P0 disk-fill fix lands now, and #54 unblocks (it is currently blocked behind this PR).

B. Rework #55 now: make StoreVariant's accounting insert authoritative, return reconciliation to startup-only. If the insert fails, the store fails and the file is removed — the request still succeeds, it just does not get cached. Then a file-without-row can only come from a crash, which is by definition followed by a startup, so the startup pass is sufficient and the periodic walk disappears entirely. Cleanest invariant and least background I/O. Cost: reopens a P0 that has passed review, needs rework plus a fresh review round, and keeps #54 blocked longer.

C. Keep periodic but decouple and lengthen it (e.g. hourly instead of reusing the 5-minute eviction interval). Small diff, cuts the recurring cost ~12x, but keeps the walk and keeps papering over the best-effort insert.

Recommendation: A. The periodic pass is a real inefficiency but not a correctness bug, and #55 is the P0 that also gates #54. I would merge it now and take option B's change as its own commit-sized unit under the 1.0.0 milestone — "make variant accounting authoritative, return reconciliation to startup-only" — where it gets a focused review instead of being bolted onto a concurrency-sensitive PR that already passed.

If you would rather have B before this merges, say so and I will dispatch the rework immediately. Leaving this merge-ready and assigned to you in the meantime.

Answering @sneak's question (issuecomment-45166). Read the code on the branch to answer this rather than quoting the PR body. ## What reconciliation is for Size accounting in this PR is a *derived* view: `UsageBytes` is two `SUM` queries over `variant_content` and `source_content`. Eviction only ever sees bytes that have a row. So the whole scheme depends on "every file on disk has an accounting row, and every row has a file" — and there is no way to make a filesystem write and a SQLite insert atomic with each other. Reconciliation is the repair pass for the gap. Concretely it fixes four kinds of drift: - **File on disk, no row** — the process died between writing the file and inserting the row, or the insert failed. Those bytes are invisible to eviction *forever*. This is the dangerous direction: it is silent, unbounded disk growth, i.e. exactly the DoS that #51 exists to close. - **Row, no file** — died between deleting the row and unlinking, or someone deleted files underneath us. Over-counts usage, so the evictor evicts real data it did not need to. - **Source blob the DB has no knowledge of** — unreachable content taking space. - **`.tmp-*` leftovers** from crashed writes. ## What "adoption" is `adoptVariantFile` (`eviction.go`) is the fix for case 1, for variants specifically. It walks the variant directory, and for a file with no `variant_content` row it inserts one, taking `size_bytes` from `stat`, `created_at`/`last_accessed_at` from the file mtime, and `content_type` from the `.meta` sidecar (falling back to `application/octet-stream`). "Adopt" = start counting this orphan file as cache usage, which also makes it eligible for eviction. Before adoption it is a file nothing will ever delete and nothing counts. It matters for variants and not sources because the two store paths differ: `StoreSource` treats its `source_content` insert as fatal (returns the error, store fails), while `StoreVariant`'s insert is deliberately **best-effort** — on failure it logs a warning and still returns success, leaving a file with no row. ## Do we need it? Three separable pieces, and the honest answer differs for each: 1. **Startup reconciliation — yes.** Crash recovery is real and there is no alternative; without it every unclean shutdown permanently leaks untracked bytes, and they accumulate across restarts. 2. **Adoption — yes, as long as any path can write a file without a row.** Today two paths can: crashes (unavoidable) and `StoreVariant`'s best-effort insert (avoidable). 3. **Re-running it on every 5-minute tick — this exists *only* because of `StoreVariant`'s best-effort insert.** That was round 1's finding: with startup-only reconciliation, an insert that fails at hour 3 of a 30-day process stays unaccounted for 30 days. Making the pass periodic bounds that to one interval. It is a patch over the best-effort insert, not an independently motivated feature. And I want to flag a cost the reviewer only partly named. `adoptVariantFile` issues one `SELECT COUNT(*) FROM variant_content WHERE cache_key = ?` **per file**, and `reconcileVariantRows` stats every tracked row — on every tick, now, not once. On a cache sized at the default (75% of free space, easily hundreds of GB, millions of variants) that is millions of SQLite queries plus a full `WalkDir` every 5 minutes, against a proxy whose README targets 1k-5k req/s. It is off the request path and shutdown-safe-ish, but `StopEviction` waits on an in-flight pass with no timeout, so it can also drag out graceful shutdown. At small cache sizes this is free; at the sizes the default config picks it is not. ## Options **A. Merge #55 as-is, file the simplification as a follow-up.** Current behavior is *correct* — two adversarial reviews confirmed it — just wasteful at scale. P0 disk-fill fix lands now, and #54 unblocks (it is currently blocked behind this PR). **B. Rework #55 now: make `StoreVariant`'s accounting insert authoritative, return reconciliation to startup-only.** If the insert fails, the store fails and the file is removed — the request still succeeds, it just does not get cached. Then a file-without-row can only come from a crash, which is by definition followed by a startup, so the startup pass is sufficient and the periodic walk disappears entirely. Cleanest invariant and least background I/O. Cost: reopens a P0 that has passed review, needs rework plus a fresh review round, and keeps #54 blocked longer. **C. Keep periodic but decouple and lengthen it** (e.g. hourly instead of reusing the 5-minute eviction interval). Small diff, cuts the recurring cost ~12x, but keeps the walk and keeps papering over the best-effort insert. **Recommendation: A.** The periodic pass is a real inefficiency but not a correctness bug, and #55 is the P0 that also gates #54. I would merge it now and take option B's change as its own commit-sized unit under the 1.0.0 milestone — "make variant accounting authoritative, return reconciliation to startup-only" — where it gets a focused review instead of being bolted onto a concurrency-sensitive PR that already passed. If you would rather have B before this merges, say so and I will dispatch the rework immediately. Leaving this `merge-ready` and assigned to you in the meantime.

clawbot referenced this pull request 2026-08-09 03:43:51 +02:00

P1: no global cap on concurrent image processing — unbounded memory under load #64

clawbot referenced this pull request 2026-08-09 03:44:03 +02:00

No request coalescing: K concurrent misses for the same variant cause K upstream fetches and K transcodes #65

clawbot referenced this pull request 2026-08-09 03:45:06 +02:00

CacheConfig.CacheTTL is configured but never read — cached variants never expire #69

clawbot referenced this pull request 2026-08-09 03:45:28 +02:00

Cache.metaCache is dead: every cache hit does open+stat+ReadFile+JSON parse instead of the in-memory lookup README promises #70

clawbot referenced this pull request 2026-08-09 03:45:56 +02:00

Silently swallowed errors: unparseable exp yields 401 instead of 400, sidecar write failures logged nowhere #72

clawbot referenced this pull request 2026-08-09 03:46:08 +02:00

Remove dead abstractions in imgcache: unimplemented Purge and three interfaces with no implementers #73

clawbot referenced this pull request 2026-08-09 03:47:11 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot referenced this pull request 2026-08-09 03:48:28 +02:00

httpfetcher: redirect-to-private-IP SSRF and semaphore release are untested #78

clawbot referenced this pull request 2026-08-09 03:48:52 +02:00

No integration test of the end-to-end image proxy flow #80

clawbot referenced this pull request 2026-08-09 03:50:01 +02:00

No pixa-specific Prometheus metrics, and metrics are not collected at all unless credentials are configured #85

clawbot referenced this pull request 2026-08-09 03:50:20 +02:00

Shutdown correctness: exit code always 0, two competing signal handlers, Sentry calls os.Exit from a goroutine #86

clawbot referenced this pull request 2026-08-09 03:50:32 +02:00

httpfetcher.hostSems grows without bound, and VariantStorage.Delete orphans the .meta sidecar #87

clawbot referenced this pull request 2026-08-09 03:51:31 +02:00

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

clawbot referenced this pull request 2026-08-09 07:03:39 +02:00

Migrations are not at the mandated internal/db/migrations/ path or filenames #96

clawbot referenced this pull request 2026-08-09 07:04:00 +02:00

Repo layout: non-canonical root files, a confusing scripts/ directory, and a stale reformatted CONVENTIONS.md #97

clawbot referenced this pull request 2026-08-09 07:04:41 +02:00

Markdown is never formatted: script/fmt covers only Go, and no prettier config exists #100

clawbot referenced this pull request 2026-08-09 07:37:54 +02:00

script/cibuild can report a green it did not earn: docker layer cache can skip make check entirely #101

clawbot referenced this pull request 2026-08-09 07:38:10 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

sneak merged commit 63fbc98e63 into main 2026-08-09 13:22:51 +02:00

sneak deleted branch feature/cache-size-eviction 2026-08-09 13:22:51 +02:00

sneak referenced this issue from a commit 2026-08-09 13:22:52 +02:00

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

clawbot referenced this pull request 2026-08-09 15:11:09 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot referenced this pull request 2026-08-09 15:37:46 +02:00

Eviction loop is not context-cancellable; shutdown ignores its deadline #102

clawbot referenced this issue from a commit 2026-08-09 15:45:59 +02:00

Merge branch 'main' into golangci-v2.12.2

clawbot referenced this issue from a commit 2026-08-09 15:45:59 +02:00

style: rename #55's white-box test files to *_internal_test.go (testpackage)

clawbot referenced this issue from a commit 2026-08-09 15:45:59 +02:00

style: mechanical lint conformance for #55's code under the canonical config

clawbot referenced this issue from a commit 2026-08-09 15:45:59 +02:00

style: convert inline error handling to plain assignment in #55's tests (noinlineerr)

clawbot referenced this issue from a commit 2026-08-09 15:45:59 +02:00

refactor: static sentinel errors for #55's config validation paths (err113)

clawbot referenced this issue from a commit 2026-08-09 15:45:59 +02:00

style: clear the last lint findings on #55's code

clawbot referenced this pull request 2026-08-09 15:49:35 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot referenced this pull request 2026-08-09 16:04:32 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot referenced this pull request 2026-08-09 16:05:55 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot referenced this pull request 2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

clawbot referenced this issue from a commit 2026-08-09 16:12:35 +02:00

docs: correct the golangci-lint entry in TODO.md

clawbot referenced this pull request 2026-08-09 16:13:01 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot referenced this pull request 2026-08-09 16:16:14 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

clawbot referenced this pull request 2026-08-09 16:18:28 +02:00

Eviction loop is not context-cancellable; shutdown ignores its deadline #102

clawbot referenced this pull request 2026-08-09 16:28:32 +02:00

Update golangci-lint to v2.12.2 with canonical config #54

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label merge-ready

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees sneak

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#55