diff --git a/TODO.md b/TODO.md index eb5461a..364f094 100644 --- a/TODO.md +++ b/TODO.md @@ -59,6 +59,17 @@ P2: security: per-origin rate limiting and the query now have each `%` written as `%25` and each `:` as `%3A`; a path and query holding neither are signed and keyed as before. Signature Specification in `README.md` says so. +- 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. `CleanExpired` deletes at most 1000 rows per statement, + so a request waits behind 1000 deletions at most, however many rows have + expired. The goroutine 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-15 the example config allowlists no host (closes #249): it allowlisted `github.com` and `user-images.githubusercontent.com`, where anyone can put an image, so a deployment set up as Getting Started says served those images to @@ -76,15 +87,6 @@ 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/imgcache/cache.go b/internal/imgcache/cache.go index f5e4e47..dc49692 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -32,6 +32,12 @@ const httpStatusOK = 200 // variant not among them is served as before, reading its .meta file. const metaCacheSize = 10000 +// cleanExpiredBatchSize is the most expired negative cache rows one +// statement of CleanExpired deletes. Each batch is a statement of its +// own, so a request waits behind one batch at most, however many rows +// have expired. +const cleanExpiredBatchSize = 1000 + // CacheConfig holds cache configuration. type CacheConfig struct { StateDir string @@ -483,20 +489,27 @@ func (c *Cache) GetSourceContent( return c.srcContent.LoadWithSize(contentHash) } -// 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. +// CleanExpired removes the negative cache rows past their expiry, one +// batch per statement, until none is left. Once ctx ends it starts no +// further batch and returns ctx's error. The eviction goroutine calls it +// at startup and on every periodic tick, so a row stays about +// 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, ` - DELETE FROM negative_cache WHERE expires_at < CURRENT_TIMESTAMP - `) - if err != nil { - return fmt.Errorf("failed to clean negative cache: %w", err) - } + for { + if ctx.Err() != nil { + return ctx.Err() + } - return nil + deleted, err := c.cleanExpiredBatch(ctx) + if err != nil { + return err + } + + if deleted < cleanExpiredBatchSize { + return nil + } + } } // Stats returns cache statistics. @@ -760,3 +773,27 @@ func (c *Cache) checkNegativeCache( return true, nil } + +// cleanExpiredBatch deletes up to cleanExpiredBatchSize negative cache +// rows past their expiry in one statement, and returns how many it +// deleted. DELETE ... LIMIT needs a SQLite build option, so the subquery +// picks the rows. +func (c *Cache) cleanExpiredBatch(ctx context.Context) (int64, error) { + result, err := c.db.ExecContext(ctx, ` + DELETE FROM negative_cache WHERE id IN ( + SELECT id FROM negative_cache + WHERE expires_at < CURRENT_TIMESTAMP + LIMIT ? + ) + `, cleanExpiredBatchSize) + if err != nil { + return 0, fmt.Errorf("failed to clean negative cache: %w", err) + } + + deleted, err := result.RowsAffected() + if err != nil { + return 0, fmt.Errorf("failed to count cleaned negative cache rows: %w", err) + } + + return deleted, nil +}