From 47246d1d1cbff955faadeade0c8ed336ed79f197 Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 15 Oct 2026 02:18:13 +0000 Subject: [PATCH] Remove expired negative cache rows in the eviction loop (closes #244) A negative cache row was deleted only when its source URL was requested again after it expired, and CleanExpired was never called, so a client asking for a new missing path each time grew the database without limit. The eviction goroutine now calls CleanExpired at startup and on every periodic tick, so a row stays at most NegativeTTL plus one interval. The goroutine also starts with the disk cache disabled, where reconciliation and eviction already do nothing, since the negative cache is used either way. Model: opus-5-5 --- TODO.md | 9 +++++++++ internal/handlers/handlers.go | 3 ++- internal/imgcache/cache.go | 10 +++++++--- internal/imgcache/eviction.go | 32 +++++++++++++++++++++++++------- 4 files changed, 43 insertions(+), 11 deletions(-) diff --git a/TODO.md b/TODO.md index 712a369..eb5461a 100644 --- a/TODO.md +++ b/TODO.md @@ -76,6 +76,15 @@ P2: security: per-origin rate limiting answered 500. Once the limit is used up, the fetcher now reads one more byte: if the body has ended it is accepted, and if there is more it is refused with `ErrResponseTooLarge`, as before. +- 2026-10-15 expired negative cache rows are removed on a schedule (closes + #244): the row of a failed fetch, which makes pixa answer 502 for that source + URL for 5 minutes, was deleted only when the same URL was requested again + after that, so a client asking for a new missing path each time added rows + that stayed for good. The eviction goroutine now calls `CleanExpired` at + startup and on each periodic tick, every 5 minutes in pixad, so a row stays at + most about 10 minutes. It runs with the disk cache disabled + (`cache_max_bytes: 0`) too, where it does nothing else, as the negative cache + is used either way. - 2026-10-13 `fit=outside` no longer makes an output above 8192 on a side (closes #257): it scales the image to cover the requested size, and the side scaled past that size was held to no maximum, so `1x8192?fit=outside` of a diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 297ba2d..da9572f 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -124,7 +124,8 @@ func (s *Handlers) initImageService() error { s.imgCache = cache // Background eviction: startup reconciliation, then periodic and - // write-pressure passes. No-op when the disk cache is disabled. + // write-pressure passes, and the removal of expired negative cache + // rows, which is all it does when the disk cache is disabled. cache.StartEviction(imgcache.DefaultEvictionInterval) // Writes the counts requests could not write by their deadline diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index 1f73019..f5e4e47 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -50,8 +50,9 @@ type CacheConfig struct { // DisableDiskCache turns the disk cache off entirely: no cache // directories are created, lookups always miss, stores are - // no-ops, and no eviction machinery runs. The config layer sets - // this when the operator configures cache_max_bytes: 0. + // no-ops, and the eviction goroutine only removes the expired + // negative cache rows. The config layer sets this when the + // operator configures cache_max_bytes: 0. DisableDiskCache bool // Logger receives accounting and eviction log output. A nil @@ -482,7 +483,10 @@ func (c *Cache) GetSourceContent( return c.srcContent.LoadWithSize(contentHash) } -// CleanExpired removes expired entries from the cache. +// CleanExpired removes the negative cache rows past their expiry. The +// eviction goroutine calls it at startup and on every periodic tick, so +// a row stays at most NegativeTTL plus one eviction interval, whether or +// not its URL is requested again. func (c *Cache) CleanExpired(ctx context.Context) error { // Clean expired negative cache entries _, err := c.db.ExecContext(ctx, ` diff --git a/internal/imgcache/eviction.go b/internal/imgcache/eviction.go index a5aff55..71f1fb0 100644 --- a/internal/imgcache/eviction.go +++ b/internal/imgcache/eviction.go @@ -437,14 +437,16 @@ func (c *Cache) notifyWritePressure() { } // StartEviction launches the background eviction goroutine, which -// reconciles the database accounting with the cache directories at -// startup and again on every periodic tick thereafter, and evicts to -// the configured limit on the given periodic interval and on -// write-pressure notifications. It is a no-op on a disabled cache or -// when already started. The goroutine outlives the caller, so it runs -// with its own context, which StopEviction cancels. +// reconciles the database accounting with the cache directories and +// removes the expired negative cache rows at startup and again on every +// periodic tick thereafter, and evicts to the configured limit on the +// given periodic interval and on write-pressure notifications. On a +// disabled cache it only removes the expired negative cache rows, as +// the negative cache is used either way. It is a no-op when already +// started. The goroutine outlives the caller, so it runs with its own +// context, which StopEviction cancels. func (c *Cache) StartEviction(interval time.Duration) { - if c.disabled || c.evictionCancel != nil { + if c.evictionCancel != nil { return } @@ -480,6 +482,7 @@ func (c *Cache) evictionLoop(ctx context.Context, interval time.Duration) { defer close(c.evictionDone) c.runReconciliationPass(ctx) + c.runCleanExpired(ctx) c.runEvictionPass(ctx) ticker := time.NewTicker(interval) @@ -500,6 +503,7 @@ func (c *Cache) evictionLoop(ctx context.Context, interval time.Duration) { // unaccounted for to one interval, on a process that is // already running this loop regardless. c.runReconciliationPass(ctx) + c.runCleanExpired(ctx) case <-c.evictionPressure: } @@ -535,6 +539,20 @@ func (c *Cache) runReconciliationPass(ctx context.Context) { } } +// runCleanExpired runs CleanExpired, logging a failure instead of +// propagating it (the loop must keep running). It does nothing once ctx +// is cancelled. +func (c *Cache) runCleanExpired(ctx context.Context) { + if ctx.Err() != nil { + return + } + + err := c.CleanExpired(ctx) + if err != nil { + c.log.Warn("negative cache cleanup failed", "error", err) + } +} + // reconcileAccounting synchronizes the database size accounting with // the actual contents of the cache directories. It runs at startup and // again on every periodic eviction tick thereafter, off the request