Split internal/imgcache into the image proxy and the cache (closes #317)

The new package internal/imageproxy holds Proxy, which was imgcache.Service,
with its Config, ImageResponse, CacheStatus and errors. NewProxy makes and
registers the counters of upstream fetches and transforms. internal/imgcache
holds the cache only, with its hit and miss counters, and imgcache.CacheConfig
is imgcache.Config. The cache reads and writes each kind it keeps with one pair
of methods: Variant and PutVariant, Source and PutSource, Failure and
PutFailure. Source replaces LookupSource followed by GetSourceContent.
imgcache.go and module.go are gone. The proxy's tests moved with it. The
cache's tests check that the reconciliation pass removes an empty or cut file
and its row. The proxy's tests remove them by hand, then check that Get fetches
or converts again.

Model: opus-5-5

internal/imgcache/cache.go

@@ -1,3 +1,10 @@
+// Package imgcache is the image cache. It keeps:
+//   - the source images fetched from upstream
+//   - the variants made from them
+//   - the negative cache: the source URLs whose fetch failed recently
+//
+// Each image is a file with a row in the database. Eviction removes the
+// least recently used images to keep the cache under its size limit.
 package imgcache
 
 import (
@@ -26,8 +33,12 @@ const maxPendingAccessTimes = 10000
 // have expired.
 const cleanExpiredBatchSize = 1000
 
-// CacheConfig holds cache configuration.
-type CacheConfig struct {
+// DefaultNegativeTTL is how long pixad keeps a failed fetch in the negative
+// cache.
+const DefaultNegativeTTL = 5 * time.Minute
+
+// Config holds cache configuration.
+type Config struct {
 	StateDir    string
 	NegativeTTL time.Duration
 
@@ -48,7 +59,7 @@ type Cache struct {
 	db         *sql.DB
 	srcContent *ContentStorage // source images by source key
 	variants   *VariantStorage // processed variants by cache key
-	config     CacheConfig
+	config     Config
 	log        *slog.Logger
 
 	// Eviction machinery. The channels are created in NewCache so
@@ -73,13 +84,10 @@ type Cache struct {
 	pendingAccessTimesDone    chan struct{}
 	pendingAccessTimesCancel  context.CancelFunc
 
-	// The counts of what the cache does, made and registered by
+	// The counts of cache hits and misses, made and registered by
 	// registerCounters (see counters.go)
-	cacheHits          prometheus.Counter
-	cacheMisses        prometheus.Counter
-	upstreamFetches    prometheus.Counter
-	upstreamFetchBytes prometheus.Counter
-	transforms         prometheus.Counter
+	cacheHits   prometheus.Counter
+	cacheMisses prometheus.Counter
 
 	// reconciliationPageSize is the most rows one read of a content table
 	// returns in the reconciliation pass. NewCache sets it to
@@ -97,7 +105,7 @@ type Cache struct {
 // metricsRegistry. pixad passes Prometheus' default registry, the one
 // /metrics serves. It makes no database call.
 func NewCache(
-	db *sql.DB, config CacheConfig, metricsRegistry prometheus.Registerer,
+	db *sql.DB, config Config, metricsRegistry prometheus.Registerer,
 ) (*Cache, error) {
 	log := config.Logger
 	if log == nil {
@@ -147,75 +155,35 @@ func NewCache(
 	return c, nil
 }
 
-// GetVariant opens a cached variant and returns a reader, its size and its
+// Variant opens a cached variant and returns a reader, its size and its
 // content type, read from the variant file's first bytes. It opens that one
-// file and makes no database call. A variant not in the cache, or any
-// variant of a disabled cache, is ErrNotFound; a variant file whose first
-// bytes are not those of an output format is another error.
-func (c *Cache) GetVariant(
+// file and makes no database call. A variant it opens has its access time
+// handed to the background writer without waiting (see touchVariant). A
+// variant not in the cache, or any variant of a disabled cache, is
+// ErrNotFound; a variant file whose first bytes are not those of an output
+// format is another error.
+func (c *Cache) Variant(
 	cacheKey imagerequest.VariantKey,
 ) (io.ReadCloser, int64, string, error) {
 	if c.config.MaxBytes == 0 {
 		return nil, 0, "", ErrNotFound
 	}
 
-	return c.variants.LoadWithContentType(cacheKey)
+	reader, size, contentType, err := c.variants.LoadWithContentType(cacheKey)
+	if err != nil {
+		return nil, 0, "", err
+	}
+
+	c.touchVariant(cacheKey)
+
+	return reader, size, contentType, nil
 }
 
-// StoreSource stores fetched source content: it writes the file named by
-// the source's key, then the source's row. When the row cannot be written
-// the file is removed and the store fails, as a file with no row is never
-// served or evicted; if that removal fails too, the next reconciliation
-// pass removes the file. On a disabled cache it is a no-op returning an
-// empty key.
-func (c *Cache) StoreSource(
-	ctx context.Context,
-	req *imagerequest.Request,
-	content io.Reader,
-) (imagerequest.SourceKey, error) {
-	if c.config.MaxBytes == 0 {
-		return "", nil
-	}
-
-	data, err := io.ReadAll(content)
-	if err != nil {
-		return "", fmt.Errorf("failed to read source content: %w", err)
-	}
-
-	key := req.Source.Key()
-
-	size, err := c.srcContent.Store(key, data)
-	if err != nil {
-		return "", fmt.Errorf("failed to store source content: %w", err)
-	}
-
-	_, err = c.db.ExecContext(ctx, `
-		INSERT INTO source_content (source_key, size_bytes)
-		VALUES (?, ?)
-		ON CONFLICT(source_key) DO UPDATE SET
-			size_bytes = excluded.size_bytes,
-			last_accessed_at = CURRENT_TIMESTAMP
-	`, string(key), size)
-	if err != nil {
-		deleteErr := c.srcContent.Delete(key)
-		if deleteErr != nil {
-			c.log.Warn("failed to remove source file whose row was not written",
-				"source_key", key, "error", deleteErr)
-		}
-
-		return "", fmt.Errorf("failed to insert source content: %w", err)
-	}
-
-	c.notifyWritePressure()
-
-	return key, nil
-}
-
-// StoreVariant stores a processed variant by its cache key and records
+// PutVariant stores a processed variant by its cache key and records
 // it in the size accounting. On a disabled cache it is a no-op. The
 // accounting insert is best-effort (the startup reconciliation pass
 // adopts any variant file that misses its accounting row).
-func (c *Cache) StoreVariant(
+func (c *Cache) PutVariant(
 	ctx context.Context, cacheKey imagerequest.VariantKey, content io.Reader,
 ) error {
 	if c.config.MaxBytes == 0 {
@@ -244,19 +212,18 @@ func (c *Cache) StoreVariant(
 	return nil
 }
 
-// LookupSource checks if we have cached source content for a request.
-// Returns the source key if found, or an empty key if not; a row whose
-// file is missing is not found. A source found has its access time
-// handed to the background writer without waiting (see
-// touchSourceContent); a disabled cache always reports no cached source.
-func (c *Cache) LookupSource(
-	ctx context.Context, req *imagerequest.Request,
-) (imagerequest.SourceKey, error) {
+// Source opens the cached source of src and returns a reader and its size
+// in bytes. A source whose row or file is missing is ErrNotFound, as is any
+// source of a disabled cache. A source it opens has its access time handed
+// to the background writer without waiting (see touchSourceContent).
+func (c *Cache) Source(
+	ctx context.Context, src imagerequest.Source,
+) (io.ReadCloser, int64, error) {
 	if c.config.MaxBytes == 0 {
-		return "", nil
+		return nil, 0, ErrNotFound
 	}
 
-	key := req.Source.Key()
+	key := src.Key()
 
 	var rowExists bool
 
@@ -264,25 +231,100 @@ func (c *Cache) LookupSource(
 		SELECT EXISTS(SELECT 1 FROM source_content WHERE source_key = ?)
 	`, string(key)).Scan(&rowExists)
 	if err != nil {
-		return "", fmt.Errorf("failed to lookup source: %w", err)
+		return nil, 0, fmt.Errorf("failed to lookup source: %w", err)
 	}
 
 	if !rowExists {
-		return "", nil
+		return nil, 0, ErrNotFound
 	}
 
-	// Verify the content file exists
-	if !c.srcContent.Exists(key) {
-		return "", nil
+	reader, size, err := c.srcContent.LoadWithSize(key)
+	if err != nil {
+		return nil, 0, err
 	}
 
 	c.touchSourceContent(key)
 
+	return reader, size, nil
+}
+
+// PutSource stores fetched source content: it writes the file named by
+// the source's key, then the source's row. When the row cannot be written
+// the file is removed and the store fails, as a file with no row is never
+// served or evicted; if that removal fails too, the next reconciliation
+// pass removes the file. On a disabled cache it is a no-op returning an
+// empty key.
+func (c *Cache) PutSource(
+	ctx context.Context,
+	src imagerequest.Source,
+	content io.Reader,
+) (imagerequest.SourceKey, error) {
+	if c.config.MaxBytes == 0 {
+		return "", nil
+	}
+
+	data, err := io.ReadAll(content)
+	if err != nil {
+		return "", fmt.Errorf("failed to read source content: %w", err)
+	}
+
+	key := src.Key()
+
+	size, err := c.srcContent.Store(key, data)
+	if err != nil {
+		return "", fmt.Errorf("failed to store source content: %w", err)
+	}
+
+	_, err = c.db.ExecContext(ctx, `
+		INSERT INTO source_content (source_key, size_bytes)
+		VALUES (?, ?)
+		ON CONFLICT(source_key) DO UPDATE SET
+			size_bytes = excluded.size_bytes,
+			last_accessed_at = CURRENT_TIMESTAMP
+	`, string(key), size)
+	if err != nil {
+		deleteErr := c.srcContent.Delete(key)
+		if deleteErr != nil {
+			c.log.Warn("failed to remove source file whose row was not written",
+				"source_key", key, "error", deleteErr)
+		}
+
+		return "", fmt.Errorf("failed to insert source content: %w", err)
+	}
+
+	c.notifyWritePressure()
+
 	return key, nil
 }
 
-// StoreNegative stores a negative cache entry for a failed fetch.
-func (c *Cache) StoreNegative(ctx context.Context, req *imagerequest.Request) error {
+// Failure reports whether src is in the negative cache. It only reads: a
+// row past its expiry is not a hit, and the eviction goroutine deletes it
+// with the other expired rows (see CleanExpired).
+func (c *Cache) Failure(
+	ctx context.Context, src imagerequest.Source,
+) (bool, error) {
+	var expiresAt time.Time
+
+	err := c.db.QueryRowContext(ctx, `
+		SELECT expires_at FROM negative_cache
+		WHERE source_host = ? AND source_path = ? AND source_query = ?
+	`, src.Host, src.Path, src.Query).Scan(&expiresAt)
+
+	if errors.Is(err, sql.ErrNoRows) {
+		return false, nil
+	}
+
+	if err != nil {
+		return false, fmt.Errorf("failed to check negative cache: %w", err)
+	}
+
+	expired := time.Now().After(expiresAt)
+
+	return !expired, nil
+}
+
+// PutFailure stores a negative cache entry for src, whose fetch failed.
+func (c *Cache) PutFailure(ctx context.Context, src imagerequest.Source) error {
 	expiresAt := time.Now().UTC().Add(c.config.NegativeTTL)
 
 	_, err := c.db.ExecContext(ctx, `
@@ -291,7 +333,7 @@ func (c *Cache) StoreNegative(ctx context.Context, req *imagerequest.Request) er
 		VALUES (?, ?, ?, ?)
 		ON CONFLICT(source_host, source_path, source_query) DO UPDATE SET
 			expires_at = excluded.expires_at
-	`, req.Source.Host, req.Source.Path, req.Source.Query, expiresAt)
+	`, src.Host, src.Path, src.Query, expiresAt)
 	if err != nil {
 		return fmt.Errorf("failed to insert negative cache: %w", err)
 	}
@@ -299,18 +341,6 @@ func (c *Cache) StoreNegative(ctx context.Context, req *imagerequest.Request) er
 	return nil
 }
 
-// GetSourceContent returns a reader for cached source content by its key,
-// and the content's size in bytes.
-func (c *Cache) GetSourceContent(
-	key imagerequest.SourceKey,
-) (io.ReadCloser, int64, error) {
-	if c.config.MaxBytes == 0 {
-		return nil, 0, ErrNotFound
-	}
-
-	return c.srcContent.LoadWithSize(key)
-}
-
 // 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
@@ -334,34 +364,15 @@ func (c *Cache) CleanExpired(ctx context.Context) error {
 	}
 }
 
-// IncrementStats counts a cache hit or miss, and an upstream fetch that read
-// fetchBytes bytes, as IncrementUpstreamFetch does. Like the other Increment
-// methods, it only adds to the cache's Prometheus counters in memory (see
-// counters.go), so it never waits for the database or fails.
-func (c *Cache) IncrementStats(hit bool, fetchBytes int64) {
+// IncrementStats counts a cache hit or miss. It only adds to the cache's
+// Prometheus counters in memory (see counters.go), so it never waits for the
+// database or fails.
+func (c *Cache) IncrementStats(hit bool) {
 	if hit {
 		c.cacheHits.Inc()
 	} else {
 		c.cacheMisses.Inc()
 	}
-
-	c.IncrementUpstreamFetch(fetchBytes)
-}
-
-// IncrementUpstreamFetch counts one upstream fetch that read fetchBytes bytes.
-// A fetch that read no bytes is not counted.
-func (c *Cache) IncrementUpstreamFetch(fetchBytes int64) {
-	if fetchBytes <= 0 {
-		return
-	}
-
-	c.upstreamFetches.Inc()
-	c.upstreamFetchBytes.Add(float64(fetchBytes))
-}
-
-// IncrementTransformCount counts one image transcoded by the image processor.
-func (c *Cache) IncrementTransformCount() {
-	c.transforms.Inc()
 }
 
 // touchVariant hands the access time of a variant served from the cache to
@@ -388,32 +399,6 @@ func (c *Cache) touchSourceContent(key imagerequest.SourceKey) {
 	}
 }
 
-// checkNegativeCache checks if a request is in the negative cache. It only
-// reads: a row past its expiry is not a hit, and the eviction goroutine
-// deletes it with the other expired rows (see CleanExpired).
-func (c *Cache) checkNegativeCache(
-	ctx context.Context, req *imagerequest.Request,
-) (bool, error) {
-	var expiresAt time.Time
-
-	err := c.db.QueryRowContext(ctx, `
-		SELECT expires_at FROM negative_cache
-		WHERE source_host = ? AND source_path = ? AND source_query = ?
-	`, req.Source.Host, req.Source.Path, req.Source.Query).Scan(&expiresAt)
-
-	if errors.Is(err, sql.ErrNoRows) {
-		return false, nil
-	}
-
-	if err != nil {
-		return false, fmt.Errorf("failed to check negative cache: %w", err)
-	}
-
-	expired := time.Now().After(expiresAt)
-
-	return !expired, 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