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