From 2afe61e3019f22ceb0fdc5558a9bd10a0c42a4c0 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 03:51:58 +0200 Subject: [PATCH 01/11] Keep max-age within an expiring image URL's lifetime (closes #63) Both image routes sent Cache-Control: public, max-age=31536000, immutable unconditionally, so a browser or proxy could keep serving an image for a year after its signed or encrypted URL had expired. max-age is now the whole seconds left until the URL expires, never negative and at most one year; a URL with no expiry keeps one year. The 304 answer uses the same value. An encrypted URL's expiry now reaches ImageRequest.Expires through ToImageRequest. immutable stays: freshness now ends no later than the URL's expiry. README.md documents the header. Model: opus-5-5 --- README.md | 7 + TODO.md | 7 + internal/encurl/encurl.go | 9 +- internal/handlers/image.go | 20 +- .../image_cache_control_internal_test.go | 203 ++++++++++++++++++ .../handlers/image_signature_internal_test.go | 6 +- internal/handlers/imageenc.go | 4 +- internal/imgcache/imgcache.go | 3 +- 8 files changed, 251 insertions(+), 8 deletions(-) create mode 100644 internal/handlers/image_cache_control_internal_test.go diff --git a/README.md b/README.md index 4765703..23fe41b 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,13 @@ than once, is refused with 400. - ``: one of `orig`, `png`, `jpeg`, `webp` - ``: `orig` or `x` (e.g. `800x600`) +An image is served with `Cache-Control: public, max-age=, immutable`. +When the URL has an expiry (an `exp`, or the TTL of an encrypted URL), +`max-age` is the whole seconds left until then, at most one year, so no browser +or proxy cache keeps the image after pixa would refuse the URL. A URL with no +expiry gets one year. `immutable` only stops a client revalidating while its +copy is fresh. + The login form (`POST /`) is limited to 5 attempts per minute per client address, counting an IPv6 client by its /64; an attempt over the limit is refused with 429 and a `Retry-After` header. Behind a reverse proxy the client diff --git a/TODO.md b/TODO.md index 56d0745..c10609b 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,13 @@ exhaustion # Completed Steps +- 2026-09-29 `max-age` never outlives an expiring URL (closes #63): both image + routes build `Cache-Control` from the request's `Expires`, which an encrypted + URL's expiry now fills too; `max-age` is one year, or the whole seconds left + until the `exp` of a `/v1/image/` URL or the expiry of an encrypted URL when + that is sooner, never negative; an allowlisted host's URL that has an `exp` + follows it too; `immutable` stays, as freshness now ends at the expiry; + documented in `README.md`. - 2026-09-28 strip metadata from processed images (closes #82): every output is exported with govips' `StripMetadata`, so it carries no EXIF, XMP, IPTC or ICC profile; the image is first turned upright with `AutoRotate` (before sizes are diff --git a/internal/encurl/encurl.go b/internal/encurl/encurl.go index 5c8819c..8815bf0 100644 --- a/internal/encurl/encurl.go +++ b/internal/encurl/encurl.go @@ -103,7 +103,8 @@ func (g *Generator) Parse(token string) (*Payload, error) { } // ToImageRequest converts the payload to an ImageRequest. -// Applies default values for omitted optional fields. +// Applies default values for omitted optional fields. An ExpiresAt of 0, a URL +// that never expires, gives the zero Expires. func (p *Payload) ToImageRequest() *imgcache.ImageRequest { format := p.Format if format == "" { @@ -120,6 +121,11 @@ func (p *Payload) ToImageRequest() *imgcache.ImageRequest { fitMode = DefaultFitMode } + var expires time.Time + if p.ExpiresAt != 0 { + expires = time.Unix(p.ExpiresAt, 0) + } + return &imgcache.ImageRequest{ SourceHost: p.SourceHost, SourcePath: p.SourcePath, @@ -131,6 +137,7 @@ func (p *Payload) ToImageRequest() *imgcache.ImageRequest { Format: format, Quality: quality, FitMode: fitMode, + Expires: expires, } } diff --git a/internal/handlers/image.go b/internal/handlers/image.go index 00b271e..561ca1d 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -220,6 +220,24 @@ func (s *Handlers) respondImageError( s.respondError(w, "internal error", http.StatusInternalServerError) } +// cacheControl returns the Cache-Control header for an image served through a +// URL that expires at expires, or never when expires is the zero time. A cache +// may keep the image for a year, but not past the URL's expiry, after which +// pixa refuses the URL. The seconds left are rounded down and never negative. +// immutable only stops revalidation while the image is fresh, so it also ends +// at the expiry. +func cacheControl(expires time.Time) string { + const oneYear = 365 * 24 * time.Hour + + maxAge := oneYear + + if !expires.IsZero() { + maxAge = min(maxAge, max(time.Until(expires), 0)) + } + + return fmt.Sprintf("public, max-age=%d, immutable", int64(maxAge/time.Second)) +} + // writeImageResponse writes headers and streams the image content, // handling conditional and HEAD requests. func (s *Handlers) writeImageResponse( @@ -235,7 +253,7 @@ func (s *Handlers) writeImageResponse( } // Cache control headers - w.Header().Set("Cache-Control", "public, max-age=31536000, immutable") + w.Header().Set("Cache-Control", cacheControl(req.Expires)) w.Header().Set("X-Pixa-Cache", string(resp.CacheStatus)) if resp.ETag != "" { diff --git a/internal/handlers/image_cache_control_internal_test.go b/internal/handlers/image_cache_control_internal_test.go new file mode 100644 index 0000000..bb6a6b9 --- /dev/null +++ b/internal/handlers/image_cache_control_internal_test.go @@ -0,0 +1,203 @@ +package handlers + +import ( + "image/color" + "log/slog" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "testing" + "testing/fstest" + "time" + + "github.com/go-chi/chi/v5" + + "sneak.berlin/go/pixa/internal/encurl" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// photoPath is the path of the JPEG that newSignedHostServer serves. +const photoPath = "/images/photo.jpg" + +// newSignedHostServer returns a router for both image routes, and the Handlers +// behind it, whose fetcher serves a JPEG at photoPath on signedHost. signedHost +// is not on the allowlist, so a /v1/image/ URL for it is served only with a +// valid signature. +func newSignedHostServer(t *testing.T) (*Handlers, http.Handler) { + t.Helper() + + cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{ + StateDir: t.TempDir(), + CacheTTL: time.Hour, + NegativeTTL: 5 * time.Minute, + }) + if err != nil { + t.Fatalf("imgcache.NewCache() error = %v", err) + } + + jpegData := generateTestJPEG(t, 100, 100, color.RGBA{255, 0, 0, 255}) + + svc, err := imgcache.NewService(&imgcache.ServiceConfig{ + Cache: cache, + Fetcher: newMockFetcher(fstest.MapFS{ + signedHost + photoPath: &fstest.MapFile{Data: jpegData}, + }), + SigningKey: testSigningKey, + }) + if err != nil { + t.Fatalf("imgcache.NewService() error = %v", err) + } + + encGen, err := encurl.NewGenerator(testSigningKey) + if err != nil { + t.Fatalf("encurl.NewGenerator() error = %v", err) + } + + h := &Handlers{ + log: slog.New(slog.DiscardHandler), + imgSvc: svc, + encGen: encGen, + } + + r := chi.NewRouter() + r.Get("/v1/image/*", h.HandleImage()) + r.Get("/v1/e/{token}/*", h.HandleImageEnc()) + + return h, r +} + +// getMaxAge sends a GET for target to srv, requires a 200, and returns the +// max-age of the response's Cache-Control header, which must read +// "public, max-age=, immutable". +func getMaxAge(t *testing.T, srv http.Handler, target string) int { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, target, nil) + rec := httptest.NewRecorder() + + srv.ServeHTTP(rec, req) + + header := rec.Header().Get("Cache-Control") + t.Logf("GET %s: %d, Cache-Control: %s", target, rec.Code, header) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK) + } + + value, hasPrefix := strings.CutPrefix(header, "public, max-age=") + value, hasSuffix := strings.CutSuffix(value, ", immutable") + + maxAge, err := strconv.Atoi(value) + if !hasPrefix || !hasSuffix || err != nil { + t.Fatalf("Cache-Control = %q, want public, max-age=, immutable", + header) + } + + return maxAge +} + +// TestHandleImage_SignedURL_MaxAgeEndsAtExp verifies that an image served +// through a signed URL expiring in 60 seconds may be cached for at most those +// 60 seconds. The lower bound of 50 shows the max-age is the time left, not 0. +func TestHandleImage_SignedURL_MaxAgeEndsAtExp(t *testing.T) { + t.Parallel() + + h, srv := newSignedHostServer(t) + + signedURL, err := h.imgSvc.GenerateSignedURL("", &imgcache.ImageRequest{ + SourceHost: signedHost, + SourcePath: photoPath, + Size: imgcache.Size{Width: 50, Height: 50}, + Format: imgcache.FormatJPEG, + }, time.Minute) + if err != nil { + t.Fatalf("GenerateSignedURL() error = %v", err) + } + + maxAge := getMaxAge(t, srv, signedURL) + if maxAge < 50 || maxAge > 60 { + t.Errorf("max-age = %d, want 50 to 60", maxAge) + } +} + +// TestHandleImage_AllowlistedHost_MaxAge verifies the max-age of an image from +// an allowlisted host, which is served without checking sig or exp. A URL with +// no exp may be cached for a year. A URL whose exp has passed is the one request +// that reaches the header after its expiry, and must get 0, never less. +func TestHandleImage_AllowlistedHost_MaxAge(t *testing.T) { + t.Parallel() + + pastExp := strconv.FormatInt(time.Now().Add(-time.Hour).Unix(), 10) + + tests := []struct { + name string + query string + wantMaxAge int + }{ + {"no exp", "", 31536000}, + {"exp already past", "?exp=" + pastExp, 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fix := setupTestHandler(t) + + r := chi.NewRouter() + r.Get("/v1/image/*", fix.handler.HandleImage()) + + maxAge := getMaxAge(t, r, + "/v1/image/"+fix.goodHost+"/images/photo.jpg/50x50.jpeg"+tt.query) + if maxAge != tt.wantMaxAge { + t.Errorf("max-age = %d, want %d", maxAge, tt.wantMaxAge) + } + }) + } +} + +// TestHandleImageEnc_MaxAge verifies that an image served through an encrypted +// URL with a 60 second TTL may be cached for at most those 60 seconds, that one +// with a two-year TTL may be cached for a year, and that one made without a +// TTL, which never expires, may be cached for a year. +func TestHandleImageEnc_MaxAge(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + expiresAt int64 + wantAtLeast int + wantAtMost int + }{ + {"60 second TTL", time.Now().Add(time.Minute).Unix(), 50, 60}, + {"two-year TTL", time.Now().Add(2 * 365 * 24 * time.Hour).Unix(), 31536000, 31536000}, + {"no TTL", 0, 31536000, 31536000}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + h, srv := newSignedHostServer(t) + + token, err := h.encGen.Generate(&encurl.Payload{ + SourceHost: signedHost, + SourcePath: photoPath, + Width: 50, + Height: 50, + Format: imgcache.FormatJPEG, + ExpiresAt: tt.expiresAt, + }) + if err != nil { + t.Fatalf("Generate() error = %v", err) + } + + maxAge := getMaxAge(t, srv, "/v1/e/"+token+"/img.jpg") + if maxAge < tt.wantAtLeast || maxAge > tt.wantAtMost { + t.Errorf("max-age = %d, want %d to %d", + maxAge, tt.wantAtLeast, tt.wantAtMost) + } + }) + } +} diff --git a/internal/handlers/image_signature_internal_test.go b/internal/handlers/image_signature_internal_test.go index c435668..c057514 100644 --- a/internal/handlers/image_signature_internal_test.go +++ b/internal/handlers/image_signature_internal_test.go @@ -14,9 +14,9 @@ import ( ) // signedHost is not on the allowlist setupTestHandler builds, so a request -// for it needs a valid signature. No image is served for it: a request that -// passes the signature check gets 502 from the failed fetch, and one that -// fails the check gets 401. +// for it needs a valid signature. setupTestHandler serves no image for it: a +// request that passes the signature check gets 502 from the failed fetch, and +// one that fails the check gets 401. const signedHost = "signed.example.com" // getImage sends a GET for target to the image route of fix and returns the diff --git a/internal/handlers/imageenc.go b/internal/handlers/imageenc.go index 8089f64..5effd8b 100644 --- a/internal/handlers/imageenc.go +++ b/internal/handlers/imageenc.go @@ -89,8 +89,8 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc { w.Header().Set("Content-Length", strconv.FormatInt(resp.ContentLength, 10)) } - // Cache headers - encrypted URLs can be cached since they're immutable - w.Header().Set("Cache-Control", "public, max-age=31536000, immutable") + // Cache headers: max-age ends at the URL's expiry + w.Header().Set("Cache-Control", cacheControl(req.Expires)) w.Header().Set("X-Pixa-Cache", string(resp.CacheStatus)) // Stream the response diff --git a/internal/imgcache/imgcache.go b/internal/imgcache/imgcache.go index 7fb069b..a6a591e 100644 --- a/internal/imgcache/imgcache.go +++ b/internal/imgcache/imgcache.go @@ -95,7 +95,8 @@ type ImageRequest struct { FitMode FitMode // Signature is the HMAC signature for non-allowlisted hosts Signature string - // Expires is the signature expiration timestamp + // Expires is when the URL expires: the exp of a signed URL, or the expiry + // of an encrypted URL; the zero time if it has none Expires time.Time // AllowHTTP indicates whether HTTP (non-TLS) is allowed for this request AllowHTTP bool -- 2.54.0 From ed3f8770e60ce83ee6187ff48563bd4a52f84f22 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 04:44:49 +0200 Subject: [PATCH 02/11] Give pixad a fixed uid and gid 65532 (closes #151) adduser took the first free uid, 1000, and the entrypoint gives a bind-mounted /var/lib/pixa to pixad, so on the host a person's login account ended up owning pixa's database and cache. The image now creates the pixad group with gid 65532 and the pixad user with uid 65532, which host login and system accounts do not use. The first-run step of "Running under upaas" in README.md names the uid and gid. Model: opus-5-5 --- Dockerfile | 8 ++++++-- README.md | 6 ++++-- TODO.md | 5 +++++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/Dockerfile b/Dockerfile index 77510a0..45027be 100644 --- a/Dockerfile +++ b/Dockerfile @@ -68,8 +68,12 @@ RUN apk add --no-cache \ COPY --from=builder /pixad /usr/local/bin/pixad COPY deploy/docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh -# Create non-root user, config directory, and data directory -RUN adduser -D -H -s /sbin/nologin pixad && \ +# Create non-root user, config directory, and data directory. pixad +# gets uid and gid 65532, which host login and system accounts do not +# use: a bind-mounted /var/lib/pixa is given to pixad, and on the host +# it must not belong to a person's account. +RUN addgroup -g 65532 pixad && \ + adduser -D -H -s /sbin/nologin -u 65532 -G pixad pixad && \ mkdir -p /var/lib/pixa /etc/pixa && \ chown pixad:pixad /var/lib/pixa diff --git a/README.md b/README.md index 23fe41b..ec673d1 100644 --- a/README.md +++ b/README.md @@ -58,8 +58,10 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app for pixa needs: `healthy`. The probe uses the port from `PORT` (default `8080`), so a port changed only in a mounted config file is not seen by it: change the port with `PORT`. -- **First run:** create the host directory. It may be owned by root: the - container gives it to its `pixad` user when it starts. +- **First run:** create the host directory, owned by root or by uid + `65532` and gid `65532`. The server runs as the container's `pixad` + user, which has that uid and gid, and the container gives the + directory to `pixad` when it starts. ## Rationale diff --git a/TODO.md b/TODO.md index c10609b..a8ec7c3 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,11 @@ exhaustion # Completed Steps +- 2026-09-29 fixed uid and gid for `pixad` (closes #151): the image creates the + `pixad` group with gid 65532 and the `pixad` user with uid 65532, instead of + the first free uid 1000, so a bind-mounted `/var/lib/pixa` given to `pixad` + is not owned on the host by a person's login account; the first-run step of + "Running under upaas" in `README.md` names the uid and gid. - 2026-09-29 `max-age` never outlives an expiring URL (closes #63): both image routes build `Cache-Control` from the request's `Expires`, which an encrypted URL's expiry now fills too; `max-age` is one year, or the whole seconds left -- 2.54.0 From e98b998cb6e644a4af2d78fc778034065d8f21b4 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 05:25:10 +0200 Subject: [PATCH 03/11] Make the cache stats count what is cached, fetched and transcoded (closes #56) Cache.Stats read request_cache and output_content, which nothing writes, so TotalItems and TotalSizeBytes were always 0. They now count source_content plus variant_content and use UsageBytes; a disabled disk cache reports 0 for both. The upstream fetch count and bytes and the transform count never moved: Get now passes the bytes it fetched (including those read before a failed body read) and counts each successful transcode. Hits, misses and these counters are written with context.WithoutCancel, so a client disconnect or the request timeout no longer loses them. The unused tables stay; metaCache is #70. Model: opus-5-5 --- TODO.md | 12 + internal/imgcache/cache.go | 39 ++- internal/imgcache/imgcache.go | 6 +- internal/imgcache/service.go | 58 +++-- internal/imgcache/stats_internal_test.go | 316 ++++++++++++++++++++++- 5 files changed, 390 insertions(+), 41 deletions(-) diff --git a/TODO.md b/TODO.md index a8ec7c3..622d81f 100644 --- a/TODO.md +++ b/TODO.md @@ -42,6 +42,18 @@ exhaustion that is sooner, never negative; an allowlisted host's URL that has an `exp` follows it too; `immutable` stays, as freshness now ends at the expiry; documented in `README.md`. +- 2026-09-28 cache stats report real numbers (closes #56): `Cache.Stats` + counts the cached source images and processed variants (`source_content` + plus `variant_content`) and takes their size from `Cache.UsageBytes`, + instead of reading `request_cache` and `output_content`, which nothing + writes; those two tables are left in the schema; a disabled disk cache + reports no items and no size. A hit is counted even when the request + context has ended. A miss is counted after it is served or fails, also + when the request context has ended by then, with the bytes it read from + upstream, so `upstream_fetch_count` and `upstream_fetch_bytes` move, + including for an upstream body that fails partway or a fetched source + that then fails the magic byte check; `transform_count` counts each image + the image processor transcodes. - 2026-09-28 strip metadata from processed images (closes #82): every output is exported with govips' `StripMetadata`, so it carries no EXIF, XMP, IPTC or ICC profile; the image is first turned upright with `AutoRotate` (before sizes are diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index 172b40e..e15b90a 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -419,19 +419,21 @@ func (c *Cache) Stats(ctx context.Context) (*CacheStats, error) { return nil, fmt.Errorf("failed to get cache stats: %w", err) } - // Get actual item count and total size from content tables - err = c.db.QueryRowContext(ctx, - `SELECT COUNT(*) FROM request_cache`, - ).Scan(&stats.TotalItems) - if err != nil { - c.log.Warn("failed to count cache items for stats", "error", err) - } + // Count and size the cached source images and processed variants. A + // disabled cache holds none, whatever rows an earlier run left. + if !c.disabled { + err = c.db.QueryRowContext(ctx, ` + SELECT (SELECT COUNT(*) FROM source_content) + + (SELECT COUNT(*) FROM variant_content) + `).Scan(&stats.TotalItems) + if err != nil { + c.log.Warn("failed to count cache items for stats", "error", err) + } - err = c.db.QueryRowContext(ctx, - `SELECT COALESCE(SUM(size_bytes), 0) FROM output_content`, - ).Scan(&stats.TotalSizeBytes) - if err != nil { - c.log.Warn("failed to sum cache size for stats", "error", err) + stats.TotalSizeBytes, err = c.UsageBytes(ctx) + if err != nil { + c.log.Warn("failed to sum cache size for stats", "error", err) + } } // Compute hit rate as a ratio @@ -481,6 +483,19 @@ func (c *Cache) IncrementStats(ctx context.Context, hit bool, fetchBytes int64) } } +// IncrementTransformCount counts one image transcoded by the image processor. +func (c *Cache) IncrementTransformCount(ctx context.Context) { + _, err := c.db.ExecContext(ctx, ` + UPDATE cache_stats + SET transform_count = transform_count + 1, + last_updated_at = CURRENT_TIMESTAMP + WHERE id = 1 + `) + if err != nil { + c.log.Warn("failed to count transform", "error", err) + } +} + // writeMetadataSidecar writes the JSON metadata sidecar of a stored source. // A failure is logged and is otherwise non-fatal; the metadata is in the // database. diff --git a/internal/imgcache/imgcache.go b/internal/imgcache/imgcache.go index a6a591e..a74e783 100644 --- a/internal/imgcache/imgcache.go +++ b/internal/imgcache/imgcache.go @@ -163,9 +163,11 @@ type ImageCache interface { // CacheStats contains cache statistics type CacheStats struct { - // TotalItems is the number of cached items + // TotalItems is the number of cached source images plus processed + // variants TotalItems int64 - // TotalSizeBytes is the total size of cached content + // TotalSizeBytes is the total size of cached source images and + // processed variants TotalSizeBytes int64 // HitCount is the number of cache hits HitCount int64 diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index 3b295c7..d85dee8 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -143,7 +143,8 @@ func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, e s.log.Error("failed to get cached variant", "key", result.CacheKey, "error", err) // Fall through to re-process } else { - s.cache.IncrementStats(ctx, true, 0) + // Counted also when the request context has ended meanwhile + s.cache.IncrementStats(context.WithoutCancel(ctx), true, 0) return &ImageResponse{ Content: reader, @@ -155,12 +156,15 @@ func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, e } } - // Cache miss - check if we have source content cached + // Cache miss - process the cached source or fetch it, then count the + // miss with the bytes it fetched from upstream, also when it failed or + // the request context has ended meanwhile cacheKey := CacheKey(req) - s.cache.IncrementStats(ctx, false, 0) + response, fetchedBytes, err := s.processFromSourceOrFetch(ctx, req, cacheKey) + + s.cache.IncrementStats(context.WithoutCancel(ctx), false, fetchedBytes) - response, err := s.processFromSourceOrFetch(ctx, req, cacheKey) if err != nil { return nil, err } @@ -268,22 +272,20 @@ func (s *Service) loadCachedSource(contentHash ContentHash) []byte { } // processFromSourceOrFetch processes an image, using cached source content -// if available. +// if available. It also returns the number of bytes fetched from upstream, +// as fetchAndProcess does, or 0 when the cached source was used. func (s *Service) processFromSourceOrFetch( ctx context.Context, req *ImageRequest, cacheKey VariantKey, -) (*ImageResponse, error) { +) (*ImageResponse, int64, error) { // Check if we have cached source content contentHash, _, err := s.cache.LookupSource(ctx, req) if err != nil { s.log.Warn("source lookup failed", "error", err) } - var ( - sourceData []byte - fetchBytes int64 - ) + var sourceData []byte if contentHash != "" { s.log.Debug("using cached source", "hash", contentHash) @@ -292,26 +294,25 @@ func (s *Service) processFromSourceOrFetch( // Fetch from upstream if we don't have source data or it's empty if len(sourceData) == 0 { - resp, err := s.fetchAndProcess(ctx, req, cacheKey) - if err != nil { - return nil, err - } - - return resp, nil + return s.fetchAndProcess(ctx, req, cacheKey) } - // Process using cached source - fetchBytes = int64(len(sourceData)) + // Process using cached source; nothing was fetched from upstream + resp, err := s.processAndStore( + ctx, req, cacheKey, sourceData, int64(len(sourceData)), + ) - return s.processAndStore(ctx, req, cacheKey, sourceData, fetchBytes) + return resp, 0, err } // fetchAndProcess fetches from upstream, processes, and caches the result. +// It also returns the number of bytes read from upstream, including when +// reading the response or a later step fails. func (s *Service) fetchAndProcess( ctx context.Context, req *ImageRequest, cacheKey VariantKey, -) (*ImageResponse, error) { +) (*ImageResponse, int64, error) { // Fetch from upstream sourceURL := req.SourceURL() @@ -330,20 +331,20 @@ func (s *Service) fetchAndProcess( } } - return nil, fmt.Errorf("upstream fetch failed: %w", err) + return nil, 0, fmt.Errorf("upstream fetch failed: %w", err) } defer func() { _ = fetchResult.Content.Close() }() // Read and validate the source content sourceData, err := io.ReadAll(fetchResult.Content) + fetchBytes := int64(len(sourceData)) + if err != nil { - return nil, fmt.Errorf("failed to read upstream response: %w", err) + return nil, fetchBytes, fmt.Errorf("failed to read upstream response: %w", err) } // Calculate download bitrate - fetchBytes := int64(len(sourceData)) - var downloadRate string if fetchResult.FetchDurationMs > 0 { @@ -368,7 +369,7 @@ func (s *Service) fetchAndProcess( // Validate magic bytes match content type err = magic.ValidateMagicBytes(sourceData, fetchResult.ContentType) if err != nil { - return nil, fmt.Errorf("content validation failed: %w", err) + return nil, fetchBytes, fmt.Errorf("content validation failed: %w", err) } // Store source content @@ -378,7 +379,9 @@ func (s *Service) fetchAndProcess( // Continue even if caching fails } - return s.processAndStore(ctx, req, cacheKey, sourceData, fetchBytes) + resp, err := s.processAndStore(ctx, req, cacheKey, sourceData, fetchBytes) + + return resp, fetchBytes, err } // processAndStore processes an image and stores the result. @@ -406,6 +409,9 @@ func (s *Service) processAndStore( processDuration := time.Since(processStart) + // Counted also when the request context has ended meanwhile + s.cache.IncrementTransformCount(context.WithoutCancel(ctx)) + // Read processed content processedData, err := io.ReadAll(processResult.Content) _ = processResult.Content.Close() diff --git a/internal/imgcache/stats_internal_test.go b/internal/imgcache/stats_internal_test.go index 5076179..94ee241 100644 --- a/internal/imgcache/stats_internal_test.go +++ b/internal/imgcache/stats_internal_test.go @@ -4,6 +4,9 @@ import ( "bytes" "context" "database/sql" + "image/color" + "io" + "io/fs" "log/slog" "math" "strings" @@ -11,6 +14,7 @@ import ( "time" "sneak.berlin/go/pixa/internal/database" + "sneak.berlin/go/pixa/internal/httpfetcher" ) func setupStatsTestDB(t *testing.T) *sql.DB { @@ -125,7 +129,7 @@ func TestStats_LogsFailedCountQueries(t *testing.T) { } _, err = db.ExecContext(t.Context(), - `DROP TABLE request_cache; DROP TABLE output_content`) + `DROP TABLE source_content; DROP TABLE variant_content`) if err != nil { t.Fatal(err) } @@ -183,3 +187,313 @@ func TestIncrementStats_LogsFailedUpdates(t *testing.T) { } } } + +// TestStats_TotalsCountSourcesAndVariants verifies that TotalItems and +// TotalSizeBytes cover the stored source images and processed variants. +func TestStats_TotalsCountSourcesAndVariants(t *testing.T) { + t.Parallel() + + cache, _ := newEvictionTestCache(t, 1<<30) + + storeEvictionTestSource(t, cache, testHostCDN, testPathCat, + bytes.Repeat([]byte{0xAA}, 1000)) + storeEvictionTestVariant(t, cache, testVariantKeyOne, + bytes.Repeat([]byte{0xAB}, 500)) + storeEvictionTestVariant(t, cache, testVariantKeyTwo, + bytes.Repeat([]byte{0xAC}, 250)) + + stats, err := cache.Stats(t.Context()) + if err != nil { + t.Fatalf("Stats() error = %v", err) + } + + if stats.TotalItems != 3 { + t.Errorf("TotalItems = %d, want 3 (1 source, 2 variants)", stats.TotalItems) + } + + if stats.TotalSizeBytes != 1750 { + t.Errorf("TotalSizeBytes = %d, want 1750 (1000+500+250)", + stats.TotalSizeBytes) + } +} + +// TestStats_DisabledCacheReportsNoItems verifies that a disabled disk cache +// reports no items and no size, even when its database still holds the +// rows of an earlier run with the disk cache enabled. +func TestStats_DisabledCacheReportsNoItems(t *testing.T) { + t.Parallel() + + enabled, _ := newEvictionTestCache(t, 1<<30) + + storeEvictionTestSource(t, enabled, testHostCDN, testPathCat, + bytes.Repeat([]byte{0xAA}, 1000)) + storeEvictionTestVariant(t, enabled, testVariantKeyOne, + bytes.Repeat([]byte{0xAB}, 500)) + + disabled, err := NewCache(enabled.db, CacheConfig{ + StateDir: t.TempDir(), + CacheTTL: time.Hour, + NegativeTTL: 5 * time.Minute, + DisableDiskCache: true, + }) + if err != nil { + t.Fatal(err) + } + + stats, err := disabled.Stats(t.Context()) + if err != nil { + t.Fatalf("Stats() error = %v", err) + } + + if stats.TotalItems != 0 || stats.TotalSizeBytes != 0 { + t.Errorf("TotalItems = %d, TotalSizeBytes = %d, want 0 and 0", + stats.TotalItems, stats.TotalSizeBytes) + } +} + +// cacheStatsCounters holds the counters of the cache_stats row, in column +// order. +type cacheStatsCounters struct { + hitCount int64 + missCount int64 + upstreamFetchCount int64 + upstreamFetchBytes int64 + transformCount int64 +} + +// readCacheStatsCounters reads the counters of the cache_stats row. +func readCacheStatsCounters(t *testing.T, cache *Cache) cacheStatsCounters { + t.Helper() + + var got cacheStatsCounters + + err := cache.db.QueryRowContext(t.Context(), ` + SELECT hit_count, miss_count, upstream_fetch_count, + upstream_fetch_bytes, transform_count + FROM cache_stats WHERE id = 1 + `).Scan(&got.hitCount, &got.missCount, &got.upstreamFetchCount, + &got.upstreamFetchBytes, &got.transformCount) + if err != nil { + t.Fatalf("failed to read cache_stats: %v", err) + } + + return got +} + +// TestService_Get_CountsStats walks Get through a miss that fetches the +// source, a hit, a miss that reuses the cached source, and two misses whose +// source cannot be used, checking every cache_stats counter after each. +func TestService_Get_CountsStats(t *testing.T) { + t.Parallel() + + svc, fixtures := SetupTestService(t) + + // NewTestFS builds the same files the test service's fetcher serves. + testFS, _ := NewTestFS(t) + + photo, err := fs.ReadFile(testFS, fixtures.GoodHostJPEG) + if err != nil { + t.Fatal(err) + } + + fake, err := fs.ReadFile(testFS, fixtures.InvalidFile) + if err != nil { + t.Fatal(err) + } + + photoBytes, fakeBytes := int64(len(photo)), int64(len(fake)) + + // want is hits, misses, upstream fetches, upstream bytes, transforms. + steps := []struct { + name string + path string + size int + wantErr bool + want cacheStatsCounters + }{ + {"miss that fetches the source", testPathPhoto, 50, false, + cacheStatsCounters{0, 1, 1, photoBytes, 1}}, + {"hit", testPathPhoto, 50, false, + cacheStatsCounters{1, 1, 1, photoBytes, 1}}, + {"miss that reuses the cached source", testPathPhoto, 25, false, + cacheStatsCounters{1, 2, 1, photoBytes, 2}}, + {"miss whose source fails the magic byte check", "/images/fake.jpg", 50, true, + cacheStatsCounters{1, 3, 2, photoBytes + fakeBytes, 2}}, + {"miss whose source is not found", "/images/nonexistent.jpg", 50, true, + cacheStatsCounters{1, 4, 2, photoBytes + fakeBytes, 2}}, + } + + for _, step := range steps { + resp, err := svc.Get(t.Context(), &ImageRequest{ + SourceHost: fixtures.GoodHost, + SourcePath: step.path, + Size: Size{Width: step.size, Height: step.size}, + Format: FormatJPEG, + Quality: 85, + FitMode: FitCover, + }) + if (err != nil) != step.wantErr { + t.Fatalf("%s: Get() error = %v, want error %t", step.name, err, step.wantErr) + } + + if err == nil { + _ = resp.Content.Close() + } + + got := readCacheStatsCounters(t, svc.cache) + if got != step.want { + t.Fatalf("after the %s: counters = %+v, want %+v", step.name, got, step.want) + } + } +} + +// fakeUpstream answers every fetch with itself as a JPEG body. The body +// serves data, then calls cancel, when set, and returns err; io.EOF ends +// the body normally. +type fakeUpstream struct { + data *bytes.Reader + cancel context.CancelFunc + err error +} + +func (u *fakeUpstream) Fetch( + context.Context, string, +) (*httpfetcher.FetchResult, error) { + return &httpfetcher.FetchResult{ + Content: io.NopCloser(u), + ContentLength: -1, + ContentType: testContentTypeJPEG, + }, nil +} + +func (u *fakeUpstream) Read(p []byte) (int, error) { + if u.data.Len() > 0 { + return u.data.Read(p) + } + + if u.cancel != nil { + u.cancel() + } + + return 0, u.err +} + +// TestService_Get_CountsInterruptedMisses checks every cache_stats counter +// after a miss whose request context ends during or after the upstream +// fetch, and after a miss whose upstream body is over the size limit. +func TestService_Get_CountsInterruptedMisses(t *testing.T) { + t.Parallel() + + photo := generateTestJPEG(t, 100, 100, color.RGBA{255, 0, 0, 255}) + half := len(photo) / 2 + + // want is hits, misses, upstream fetches, upstream bytes, transforms. + tests := []struct { + name string + served int // bytes of the photo the upstream body serves + cancel bool // whether the body then ends the request context + readErr error // what the body then returns + wantErr bool + want cacheStatsCounters + }{ + {"request context ends during the fetch", half, true, context.Canceled, true, + cacheStatsCounters{0, 1, 1, int64(half), 0}}, + {"request context ends after the fetch", len(photo), true, io.EOF, false, + cacheStatsCounters{0, 1, 1, int64(len(photo)), 1}}, + {"upstream body over the size limit", half, false, + httpfetcher.ErrResponseTooLarge, true, + cacheStatsCounters{0, 1, 1, int64(half), 0}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + svc, fixtures := SetupTestService(t) + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + upstream := &fakeUpstream{ + data: bytes.NewReader(photo[:tc.served]), + err: tc.readErr, + } + if tc.cancel { + upstream.cancel = cancel + } + + svc.fetcher = upstream + + resp, err := svc.Get(ctx, &ImageRequest{ + SourceHost: fixtures.GoodHost, + SourcePath: testPathPhoto, + Size: Size{Width: 50, Height: 50}, + Format: FormatJPEG, + Quality: 85, + FitMode: FitCover, + }) + t.Logf("Get() error = %v", err) + + if (err != nil) != tc.wantErr { + t.Fatalf("Get() error = %v, want error %t", err, tc.wantErr) + } + + if err == nil { + _ = resp.Content.Close() + } + + got := readCacheStatsCounters(t, svc.cache) + if got != tc.want { + t.Errorf("counters = %+v, want %+v", got, tc.want) + } + }) + } +} + +// TestService_Get_CountsHitAfterRequestEnds checks every cache_stats counter +// after a hit served with a request context that has already ended: only +// the hit count moves. +func TestService_Get_CountsHitAfterRequestEnds(t *testing.T) { + t.Parallel() + + svc, fixtures := SetupTestService(t) + + req := &ImageRequest{ + SourceHost: fixtures.GoodHost, + SourcePath: testPathPhoto, + Size: Size{Width: 50, Height: 50}, + Format: FormatJPEG, + Quality: 85, + FitMode: FitCover, + } + + // A first request caches the variant. + resp, err := svc.Get(t.Context(), req) + if err != nil { + t.Fatalf("first Get() error = %v", err) + } + + _ = resp.Content.Close() + + want := readCacheStatsCounters(t, svc.cache) + want.hitCount++ + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + resp, err = svc.Get(ctx, req) + if err != nil { + t.Fatalf("Get() with an ended request context: error = %v", err) + } + + _ = resp.Content.Close() + + if resp.CacheStatus != CacheHit { + t.Fatalf("CacheStatus = %v, want %v", resp.CacheStatus, CacheHit) + } + + got := readCacheStatsCounters(t, svc.cache) + if got != want { + t.Errorf("counters = %+v, want %+v", got, want) + } +} -- 2.54.0 From 1b920fe000b06a139a7b0eb9081708441b4655f6 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 06:05:44 +0200 Subject: [PATCH 04/11] Correct trusted_proxies advice and state signature padding (closes #150) The README told operators to set trusted_proxies to the proxy's own address. A proxy on the Docker host that connects over 127.0.0.1 reaches pixa from the Docker network's gateway, so that advice made pixa count every user as one client for the login limit. The login-limit paragraph, the trusted_proxies entry and config.example.yml now say to use the address pixa sees for requests through the proxy, that a proxy connecting through another host address is seen with that address, and how to read it from the request log. The signature section now says sig is base64url with the = padding kept, since pixa compares it exactly, and shows the example's sig for a stated key, computed with pixa's signer. Model: opus-5-5 --- README.md | 53 ++++++++++++++++++++++++++++++++-------------- TODO.md | 8 +++++++ config.example.yml | 8 ++++++- 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index ec673d1..4102390 100644 --- a/README.md +++ b/README.md @@ -112,13 +112,19 @@ copy is fresh. The login form (`POST /`) is limited to 5 attempts per minute per client address, counting an IPv6 client by its /64; an attempt over the limit is refused with 429 and a `Retry-After` header. Behind a reverse proxy the client -address comes from `X-Forwarded-For` only when the proxy's address is in -`trusted_proxies`; otherwise all users behind the proxy are counted as one -client. With the default `trusted_proxies` (the RFC 1918 ranges), a client -with a private address can choose the address it is counted by through its own -`X-Forwarded-For`, whether it connects directly or through the proxy, because -its own address is trusted too. Setting `trusted_proxies` to the proxy's own -address closes this. +address comes from `X-Forwarded-For` only when the address pixa sees for +requests that come through the proxy is in `trusted_proxies`; otherwise all +users behind the proxy are counted as one client. That address is not always +the proxy's own: a proxy on the Docker host that connects to pixa over +`127.0.0.1` is seen as the gateway of the container's Docker network, such as +`172.17.0.1` on the default bridge, and one that connects through another of the +host's addresses is seen with that address. To be sure, read it as `remoteIP` in +pixa's request log while it is not in `trusted_proxies` (see `trusted_proxies` +under Configuration). With the default `trusted_proxies` (the RFC 1918 ranges), +a client with a private address can choose the address it is counted by through +its own `X-Forwarded-For`, whether it connects directly or through the proxy, +because its own address is trusted too. Setting `trusted_proxies` to only the +address pixa sees for requests that come through the proxy closes this. ### Image Metadata @@ -172,19 +178,27 @@ Where: outside), or `cover` when the URL has no `fit`; a request whose `fit` is anything else, an empty `fit=` included, is refused with 400 -**Example:** resize `https://cdn.example.com/photos/cat.jpg` to 800x600 -WebP with expiration 1704067200, default quality and fit: +The URL's `sig` is the HMAC-SHA256 result in base64url (the URL-safe alphabet +of RFC 4648) with the trailing `=` padding kept, 44 characters in all. pixa +compares it exactly, so a signature encoded without padding, as Node's +`base64url` and Go's `base64.RawURLEncoding` do, is refused with 401. + +**Example:** with the signing key `example-signing-key-for-documentation`, +resize `https://cdn.example.com/photos/cat.jpg` to 800x600 WebP with +expiration 1704067200, default quality and fit: 1. Build input: `cdn.example.com:/photos/cat.jpg::800:600:webp:1704067200:85:cover` -2. Compute HMAC-SHA256 with your secret key -3. Base64URL-encode the result +2. Compute HMAC-SHA256 of it with the signing key +3. Base64URL-encode the result, keeping the `=` padding: + `-ay7KHpfqmtIGbibDGbUuBDkymi-Ymdn0NkC6j5EJag=` 4. URL: - `/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp?sig=&exp=1704067200` + `/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp?sig=-ay7KHpfqmtIGbibDGbUuBDkymi-Ymdn0NkC6j5EJag=&exp=1704067200` For the same image at quality 40 with fit `contain`, the input ends in -`:40:contain` and the URL is -`/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp?sig=&exp=1704067200&q=40&fit=contain`. +`:40:contain`, the signature is `5IwXUx6vf7yefhaUvFzgXZvG2o0Df4RJxPTK3pKq5VU=`, +and the URL is +`/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp?sig=5IwXUx6vf7yefhaUvFzgXZvG2o0Df4RJxPTK3pKq5VU=&exp=1704067200&q=40&fit=contain`. **Allowlist patterns:** @@ -246,8 +260,15 @@ Key settings in more detail: `172.16.0.0/12`, `192.168.0.0/16`), since pixa is deployed behind a proxy on a private network; an explicitly empty list (`[]`) trusts no one, and an explicit list replaces the default. An invalid CIDR aborts - startup. Set this to your proxy's address range if it is not already - covered by the defaults + startup. Set this to the address pixa sees for requests that come through + your proxy, such as `172.17.0.1/32`, when the defaults do not cover it, or + to trust nothing else (see the login limit under Routes). For a proxy on + the Docker host that connects to pixa over `127.0.0.1`, that address is the + gateway of the container's Docker network (`172.17.0.1` on the default + bridge), not the proxy's own address; a proxy that connects through another of + the host's addresses is seen with that address. To be sure which address it + is, set this to `[]` (or `PIXA_TRUSTED_PROXIES` to empty), send a request + through the proxy, and read `remoteIP` in pixa's request log line for it - `upstream_fetch_timeout` — timeout for origin requests - `upstream_max_response_size` — max origin response size - `downstream_timeout` — client response timeout diff --git a/TODO.md b/TODO.md index 622d81f..5c0ff89 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,14 @@ exhaustion # Completed Steps +- 2026-09-29 `trusted_proxies` advice and signature padding in `README.md` + (closes #150): the login-limit paragraph, the `trusted_proxies` entry and + `config.example.yml` say to set `trusted_proxies` to the address pixa sees for + requests that come through the proxy, which the request log shows as + `remoteIP` while it is not trusted; for a proxy on the Docker host that + connects over `127.0.0.1` that is the Docker network's gateway, not the + proxy's own address; the signature section says `sig` is base64url with the + `=` padding kept, and gives the example's `sig` for a stated signing key. - 2026-09-29 fixed uid and gid for `pixad` (closes #151): the image creates the `pixad` group with gid 65532 and the `pixad` user with uid 65532, instead of the first free uid 1000, so a bind-mounted `/var/lib/pixa` given to `pixad` diff --git a/config.example.yml b/config.example.yml index 8df6a35..f92b752 100644 --- a/config.example.yml +++ b/config.example.yml @@ -50,7 +50,13 @@ allowlist_hosts: # 172.16.0.0/12, 192.168.0.0/16), since pixa is deployed behind a proxy on # a private network. An explicitly empty list ([]) trusts no one; an # explicit list replaces the default. An invalid CIDR aborts startup. -# Uncomment to override the defaults with your proxy's address range. +# Uncomment to override the defaults with the address pixa sees for +# requests that come through your proxy. That is not always the proxy's own +# address: a proxy on the Docker host that connects over 127.0.0.1 is seen +# as the gateway of the container's Docker network (172.17.0.1 on the +# default bridge), and one that connects through another host address is +# seen with that address. To be sure, look it up in the request log as the +# trusted_proxies entry in README.md describes. # trusted_proxies: # - 10.0.0.0/8 # - 2001:db8::/32 -- 2.54.0 From 46a55ec15da19e8939e4bb20d0bf732d25cfff01 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 07:42:05 +0200 Subject: [PATCH 05/11] Move migrations to internal/db/migrations (closes #96) REPO_POLICIES.md puts migrations in internal/db/migrations/ as 000_migration.sql and 001_schema.sql. The two files move there with their contents unchanged. go:embed cannot reach outside its own package, so internal/db/migrations has a small package that embeds them, and internal/database reads them through its FS(). The database package stays where CONVENTIONS.md puts it; moving it would change existing test files in other packages. The version still comes from the filename prefix, so a database that has recorded versions 0 and 1 runs neither again. A new test applies the migrations twice to one database file and checks that the second run applies nothing. Model: opus-5-5 --- TODO.md | 7 +++ internal/database/database.go | 32 ++++++----- internal/database/migrations_internal_test.go | 54 +++++++++++++++++++ .../migrations/000_migration.sql} | 0 .../migrations/001_schema.sql} | 0 internal/db/migrations/migrations.go | 15 ++++++ 6 files changed, 91 insertions(+), 17 deletions(-) create mode 100644 internal/database/migrations_internal_test.go rename internal/{database/schema/000.sql => db/migrations/000_migration.sql} (100%) rename internal/{database/schema/001_initial_schema.sql => db/migrations/001_schema.sql} (100%) create mode 100644 internal/db/migrations/migrations.go diff --git a/TODO.md b/TODO.md index 5c0ff89..8f07fa8 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,13 @@ exhaustion # Completed Steps +- 2026-09-29 migrations at the path `REPO_POLICIES.md` sets (closes #96): the + migration files moved, contents unchanged, from `internal/database/schema/` + to `internal/db/migrations/` as `000_migration.sql` and `001_schema.sql`; the + `internal/db/migrations` package embeds them and `internal/database` reads + them through its `FS()`; the `internal/database` package itself stays; the + version still comes from the filename prefix, so a database that has recorded + versions 0 and 1 runs neither again. - 2026-09-29 `trusted_proxies` advice and signature padding in `README.md` (closes #150): the login-limit paragraph, the `trusted_proxies` entry and `config.example.yml` say to set `trusted_proxies` to the address pixa sees for diff --git a/internal/database/database.go b/internal/database/database.go index f29560c..ab59f75 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -4,9 +4,9 @@ package database import ( "context" "database/sql" - "embed" "errors" "fmt" + "io/fs" "log/slog" "path/filepath" "sort" @@ -15,14 +15,12 @@ import ( "go.uber.org/fx" "sneak.berlin/go/pixa/internal/config" + "sneak.berlin/go/pixa/internal/db/migrations" "sneak.berlin/go/pixa/internal/logger" _ "modernc.org/sqlite" // SQLite driver registration ) -//go:embed schema/*.sql -var schemaFS embed.FS - // bootstrapVersion is the migration that creates the schema_migrations // table itself. It is applied before the normal migration loop. const bootstrapVersion = 0 @@ -113,29 +111,29 @@ func New(lc fx.Lifecycle, params Params) (*Database, error) { return s, nil } -// collectMigrations reads the embedded schema directory and returns +// collectMigrations reads the embedded migrations directory and returns // migration filenames sorted lexicographically. func collectMigrations() ([]string, error) { - entries, err := schemaFS.ReadDir("schema") + entries, err := fs.ReadDir(migrations.FS(), ".") if err != nil { - return nil, fmt.Errorf("failed to read schema directory: %w", err) + return nil, fmt.Errorf("failed to read migrations directory: %w", err) } - var migrations []string + var filenames []string for _, entry := range entries { if !entry.IsDir() && strings.HasSuffix(entry.Name(), ".sql") { - migrations = append(migrations, entry.Name()) + filenames = append(filenames, entry.Name()) } } - sort.Strings(migrations) + sort.Strings(filenames) - return migrations, nil + return filenames, nil } // bootstrapMigrationsTable ensures the schema_migrations table exists -// by applying 000.sql if the table is missing. +// by applying 000_migration.sql if the table is missing. func bootstrapMigrationsTable(ctx context.Context, db *sql.DB, log *slog.Logger) error { var tableExists int @@ -150,9 +148,9 @@ func bootstrapMigrationsTable(ctx context.Context, db *sql.DB, log *slog.Logger) return nil } - content, err := schemaFS.ReadFile("schema/000.sql") + content, err := fs.ReadFile(migrations.FS(), "000_migration.sql") if err != nil { - return fmt.Errorf("failed to read bootstrap migration 000.sql: %w", err) + return fmt.Errorf("failed to read bootstrap migration 000_migration.sql: %w", err) } if log != nil { @@ -177,12 +175,12 @@ func ApplyMigrations(ctx context.Context, db *sql.DB, log *slog.Logger) error { return err } - migrations, err := collectMigrations() + filenames, err := collectMigrations() if err != nil { return err } - for _, migration := range migrations { + for _, migration := range filenames { version, parseErr := ParseMigrationVersion(migration) if parseErr != nil { return parseErr @@ -208,7 +206,7 @@ func ApplyMigrations(ctx context.Context, db *sql.DB, log *slog.Logger) error { } // Read and apply migration. - content, readErr := schemaFS.ReadFile(filepath.Join("schema", migration)) + content, readErr := fs.ReadFile(migrations.FS(), migration) if readErr != nil { return fmt.Errorf("failed to read migration %s: %w", migration, readErr) } diff --git a/internal/database/migrations_internal_test.go b/internal/database/migrations_internal_test.go new file mode 100644 index 0000000..6fb7282 --- /dev/null +++ b/internal/database/migrations_internal_test.go @@ -0,0 +1,54 @@ +package database + +import ( + "bytes" + "database/sql" + "log/slog" + "path/filepath" + "strings" + "testing" + + _ "modernc.org/sqlite" // SQLite driver registration +) + +// TestApplyMigrations_SecondRunAppliesNothing applies the migrations twice +// to one database file, as happens when pixad starts again on the database +// it created, and checks that the second run applies none of them. +// ApplyMigrations logs a message starting with "applying" before it runs +// any migration, the bootstrap one included. +func TestApplyMigrations_SecondRunAppliesNothing(t *testing.T) { + t.Parallel() + + ctx := t.Context() + + db, err := sql.Open("sqlite", filepath.Join(t.TempDir(), "state.sqlite3")) + if err != nil { + t.Fatalf("failed to open test db: %v", err) + } + + t.Cleanup(func() { _ = db.Close() }) + + var firstLog bytes.Buffer + + err = ApplyMigrations(ctx, db, slog.New(slog.NewTextHandler(&firstLog, nil))) + if err != nil { + t.Fatalf("first ApplyMigrations failed: %v", err) + } + + if !strings.Contains(firstLog.String(), "applying") { + t.Fatalf("first ApplyMigrations logged no applied migration:\n%s", + firstLog.String()) + } + + var secondLog bytes.Buffer + + err = ApplyMigrations(ctx, db, slog.New(slog.NewTextHandler(&secondLog, nil))) + if err != nil { + t.Fatalf("second ApplyMigrations failed: %v", err) + } + + if strings.Contains(secondLog.String(), "applying") { + t.Errorf("second ApplyMigrations ran a migration again:\n%s", + secondLog.String()) + } +} diff --git a/internal/database/schema/000.sql b/internal/db/migrations/000_migration.sql similarity index 100% rename from internal/database/schema/000.sql rename to internal/db/migrations/000_migration.sql diff --git a/internal/database/schema/001_initial_schema.sql b/internal/db/migrations/001_schema.sql similarity index 100% rename from internal/database/schema/001_initial_schema.sql rename to internal/db/migrations/001_schema.sql diff --git a/internal/db/migrations/migrations.go b/internal/db/migrations/migrations.go new file mode 100644 index 0000000..62402ad --- /dev/null +++ b/internal/db/migrations/migrations.go @@ -0,0 +1,15 @@ +// Package migrations provides the embedded SQL migration files. +package migrations + +import ( + "embed" + "io/fs" +) + +//go:embed *.sql +var files embed.FS + +// FS returns the embedded filesystem containing the migration files. +func FS() fs.FS { + return files +} -- 2.54.0 From 56217cbf4ab25350c63f81401153dbf936601626 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 08:49:18 +0200 Subject: [PATCH 06/11] Add the four settings the README documented but pixa lacked (closes #61) README.md documented access_control_allow_origin, upstream_fetch_timeout, upstream_max_response_size and downstream_timeout, but pixa did not know them, so a config following the README aborted startup. Each is now a setting with its PIXA_ variable, defaulting to the value that was fixed in the code: *, 30s, 50 MiB and 60s. Durations are Go duration strings and must be positive; the size is whole bytes, at most 1 GiB. The origin is * or one http or https origin written exactly as a browser sends it; anything else aborts startup. downstream_timeout sets both the server's write timeout and the per-request timeout. The owner approved the edits to existing tests. Model: opus-5-5 --- README.md | 24 +- TODO.md | 12 +- config.example.yml | 25 ++ internal/config/config.go | 176 ++++++++++++- .../config/config_validation_internal_test.go | 242 ++++++++++++++++++ internal/config/env_internal_test.go | 33 +++ internal/handlers/handlers.go | 2 + internal/middleware/middleware.go | 2 +- .../middleware/middleware_internal_test.go | 47 ++++ internal/server/http.go | 3 +- internal/server/http_internal_test.go | 10 +- .../server/login_rate_limit_internal_test.go | 11 +- internal/server/routes.go | 2 +- 13 files changed, 571 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 4102390..7a3a013 100644 --- a/README.md +++ b/README.md @@ -235,6 +235,10 @@ variables set by the file's `env:` section are checked the same way. | `PIXA_TRUSTED_PROXIES` | `trusted_proxies` | CIDR ranges of proxies whose `X-Forwarded-For` is believed; default RFC 1918 | | `PIXA_ALLOW_HTTP` | `allow_http` | Allow plain-HTTP upstreams, for testing only; default `false` | | `PIXA_UPSTREAM_CONNECTIONS_PER_HOST` | `upstream_connections_per_host` | Concurrent connections per upstream host; default `20` | +| `PIXA_UPSTREAM_FETCH_TIMEOUT` | `upstream_fetch_timeout` | Time allowed for one fetch from an upstream host; default `30s` | +| `PIXA_UPSTREAM_MAX_RESPONSE_SIZE` | `upstream_max_response_size` | Largest upstream response accepted, in bytes; default 50 MiB | +| `PIXA_DOWNSTREAM_TIMEOUT` | `downstream_timeout` | Time allowed for answering one client request; default `60s` | +| `PIXA_ACCESS_CONTROL_ALLOW_ORIGIN` | `access_control_allow_origin` | CORS origin allowed to read responses: `*` or one origin; default `*` | | `PIXA_METRICS_USERNAME` | `metrics.username` | Username for `/metrics`, which is served only when both are set | | `PIXA_METRICS_PASSWORD` | `metrics.password` | Password for `/metrics`; set together with the username | | `PIXA_SENTRY_DSN` | `sentry_dsn` | Sentry DSN for error reporting; empty disables it | @@ -243,7 +247,14 @@ variables set by the file's `env:` section are checked the same way. Key settings in more detail: -- `access_control_allow_origin` — CORS origin +- `access_control_allow_origin` — the origin a browser lets read pixa's + responses, sent as the CORS `Access-Control-Allow-Origin` header: `*`, the + default, is any site; otherwise one `http` or `https` origin such as + `https://example.com`, whose host is a lowercase host name (letters, + digits, hyphens and dots, with a letter in its last part) or an IP address + (IPv6 in brackets, in its shortest form), with an optional port 1-65535 + that has no leading zero and is not the scheme's default. Any other value, + including another scheme such as a browser extension's, aborts startup - `allowlist_hosts` — list of allowed upstream hosts - `blocked_networks` — list of CIDR ranges to refuse for SSRF protection, added to the always-enforced built-in ranges (loopback, private, @@ -269,9 +280,14 @@ Key settings in more detail: the host's addresses is seen with that address. To be sure which address it is, set this to `[]` (or `PIXA_TRUSTED_PROXIES` to empty), send a request through the proxy, and read `remoteIP` in pixa's request log line for it -- `upstream_fetch_timeout` — timeout for origin requests -- `upstream_max_response_size` — max origin response size -- `downstream_timeout` — client response timeout +- `upstream_fetch_timeout` — time allowed for one fetch from an upstream + host, as a duration such as `30s` (the default) or `2m` +- `upstream_max_response_size` — largest upstream response accepted, in + bytes; default `52428800` (50 MiB). It also limits the image data pixa + decodes +- `downstream_timeout` — time allowed for answering one client request, as a + duration; default `60s`. The upstream fetch counts toward it, so keep it + longer than `upstream_fetch_timeout` - `signing_key` — HMAC secret for URL signatures - `cache_max_bytes` — disk cache size limit in bytes; `0` disables the disk cache entirely; omitted defaults to 75% of the free space on diff --git a/TODO.md b/TODO.md index 8f07fa8..e6ad439 100644 --- a/TODO.md +++ b/TODO.md @@ -57,6 +57,17 @@ exhaustion that is sooner, never negative; an allowlisted host's URL that has an `exp` follows it too; `immutable` stays, as freshness now ends at the expiry; documented in `README.md`. +- 2026-09-28 add the four settings `README.md` documented but pixa did not + have, which aborted startup as unknown keys (closes #61): + `access_control_allow_origin` (default `*`, the CORS origin), + `upstream_fetch_timeout` (default `30s`), `upstream_max_response_size` + (default 50 MiB) and `downstream_timeout` (default `60s`, both the + server's write timeout and the per-request timeout); each has a + `PIXA_` variable; durations are positive Go duration strings, the size a + whole number of bytes up to 1 GiB, the origin `*` or one `http` or + `https` origin as `README.md` describes it; an invalid value + aborts startup naming the key and the value; documented in + `config.example.yml` and `README.md`. - 2026-09-28 cache stats report real numbers (closes #56): `Cache.Stats` counts the cached source images and processed variants (`source_content` plus `variant_content`) and takes their size from `Cache.UsageBytes`, @@ -306,7 +317,6 @@ exhaustion - X-Request-ID propagation - P2: auto format selection (format=auto based on Accept header) - P2: configuration - - add all configuration options from README - YAML config file support - P2: operational - optional Sentry error reporting diff --git a/config.example.yml b/config.example.yml index f92b752..27c2422 100644 --- a/config.example.yml +++ b/config.example.yml @@ -8,6 +8,10 @@ # this file's env: section is set while the file loads, so it overrides # both the environment the process was started with and this file's own # key. +# +# Durations are Go duration strings such as 30s or 2m and must be +# positive; a bare number has no unit and aborts startup. Sizes are a +# whole number of bytes. # Server settings port: 8080 @@ -67,6 +71,27 @@ allow_http: false # Maximum concurrent connections per upstream host (default: 20) upstream_connections_per_host: 20 +# Time allowed for one fetch from an upstream host (default: 30s) +upstream_fetch_timeout: 30s + +# Largest upstream response accepted, in bytes, at most 1073741824 +# (1 GiB) (default: 52428800, 50 MiB) +upstream_max_response_size: 52428800 + +# Time allowed for answering one client request, the upstream fetch +# included, so keep it longer than upstream_fetch_timeout (default: 60s) +downstream_timeout: 60s + +# The origin a browser lets read pixa's responses, sent as the CORS +# Access-Control-Allow-Origin header: "*" (the default) is any site; +# otherwise one http or https origin such as https://example.com, whose +# host is a lowercase host name (letters, digits, hyphens and dots, with a +# letter in its last part) or an IP address (IPv6 in brackets, in its +# shortest form), with an optional port 1-65535 that has no leading zero +# and is not the scheme's default. Any other value, including another +# scheme such as a browser extension's, aborts startup. +access_control_allow_origin: "*" + # Maximum disk cache size in bytes. Explicit values are used exactly as # given; 0 disables the disk cache entirely (every request fetches and # processes uncached). When omitted, the default is 75% of the free diff --git a/internal/config/config.go b/internal/config/config.go index 3d948be..5c9504f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -13,6 +13,7 @@ import ( "sort" "strconv" "strings" + "time" "git.eeqj.de/sneak/smartconfig" "go.uber.org/fx" @@ -24,6 +25,10 @@ const ( DefaultPort = 8080 DefaultStateDir = "/var/lib/pixa" DefaultUpstreamConnectionsPerHost = 20 + DefaultAccessControlAllowOrigin = "*" + DefaultUpstreamFetchTimeout = 30 * time.Second + DefaultUpstreamMaxResponseSize = 50 << 20 // 50 MiB + DefaultDownstreamTimeout = 60 * time.Second ) // Configuration key names. @@ -44,6 +49,10 @@ const ( keyCacheMaxBytes = "cache_max_bytes" keyBlockedNetworks = "blocked_networks" keyTrustedProxies = "trusted_proxies" + keyAccessControlAllowOrigin = "access_control_allow_origin" + keyUpstreamFetchTimeout = "upstream_fetch_timeout" + keyUpstreamMaxResponseSize = "upstream_max_response_size" + keyDownstreamTimeout = "downstream_timeout" ) // placeholderSigningKey is the dummy signing_key shipped in @@ -69,6 +78,7 @@ var ( errEmptyEntry = errors.New("contains an empty entry") errNotAValidURL = errors.New("not a valid URL") errPortOutOfRange = errors.New("outside the valid port range") + errSizeOutOfRange = errors.New("outside the accepted range") errTooFewConnections = errors.New("must be at least 1") errValueTooShort = errors.New("value too short") errPlaceholderKey = errors.New( @@ -86,6 +96,10 @@ var ( errNotBareHostname = errors.New( "must be a bare hostname without scheme, path, or whitespace") errNoHostnameLabels = errors.New("contains no hostname labels") + errNotADuration = errors.New("not a duration such as 30s or 2m") + errMustBePositive = errors.New("must be positive") + errNotAnOrigin = errors.New( + `not "*" or an origin such as https://example.com`) ) // Params defines dependencies for Config. @@ -112,6 +126,23 @@ type Config struct { AllowHTTP bool // Allow non-TLS upstream (testing only) UpstreamConnectionsPerHost int // Max concurrent connections per upstream host + // UpstreamFetchTimeout is the time allowed for one fetch from an + // upstream host. UpstreamMaxResponseSize is the largest upstream + // response accepted, in bytes, and also the image processor's input + // limit. + UpstreamFetchTimeout time.Duration + UpstreamMaxResponseSize int64 + + // AccessControlAllowOrigin is the origin the CORS middleware allows + // to read responses: "*" for any, or one origin such as + // https://example.com. + AccessControlAllowOrigin string + + // DownstreamTimeout bounds how long answering one client request may + // take. It is both the HTTP server's write timeout and the deadline + // of the per-request timeout middleware. + DownstreamTimeout time.Duration + // BlockedNetworks are operator-supplied CIDR ranges to refuse in // addition to the built-in SSRF blocklist. Enforced by the upstream // fetcher's dialer; the built-in ranges always apply. @@ -240,6 +271,14 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { AllowHTTP: loader.boolVal(keyAllowHTTP, false), UpstreamConnectionsPerHost: loader.intVal( keyUpstreamConnectionsPerHost, DefaultUpstreamConnectionsPerHost), + UpstreamFetchTimeout: loader.durationVal( + keyUpstreamFetchTimeout, DefaultUpstreamFetchTimeout), + UpstreamMaxResponseSize: loader.int64Val( + keyUpstreamMaxResponseSize, DefaultUpstreamMaxResponseSize), + AccessControlAllowOrigin: loader.stringVal( + keyAccessControlAllowOrigin, DefaultAccessControlAllowOrigin), + DownstreamTimeout: loader.durationVal( + keyDownstreamTimeout, DefaultDownstreamTimeout), CacheMaxBytes: loader.int64Val(keyCacheMaxBytes, 0), BlockedNetworks: blockedNetworks, TrustedProxies: trustedProxies, @@ -354,7 +393,8 @@ func isKnownConfigKey(key string) bool { case keyDebug, keyMaintenanceMode, keyPort, keyStateDir, keySentryDSN, keyDBURL, keyMetrics, keySigningKey, keyAllowlistHosts, keyAllowHTTP, keyUpstreamConnectionsPerHost, keyCacheMaxBytes, keyBlockedNetworks, - keyTrustedProxies, "env": + keyTrustedProxies, keyAccessControlAllowOrigin, keyUpstreamFetchTimeout, + keyUpstreamMaxResponseSize, keyDownstreamTimeout, "env": return true } @@ -382,6 +422,10 @@ func envVarNames() map[string]string { keyCacheMaxBytes: "PIXA_CACHE_MAX_BYTES", keyBlockedNetworks: "PIXA_BLOCKED_NETWORKS", keyTrustedProxies: "PIXA_TRUSTED_PROXIES", + keyAccessControlAllowOrigin: "PIXA_ACCESS_CONTROL_ALLOW_ORIGIN", + keyUpstreamFetchTimeout: "PIXA_UPSTREAM_FETCH_TIMEOUT", + keyUpstreamMaxResponseSize: "PIXA_UPSTREAM_MAX_RESPONSE_SIZE", + keyDownstreamTimeout: "PIXA_DOWNSTREAM_TIMEOUT", } } @@ -535,6 +579,11 @@ func (c *Config) validate() error { settingName(keyCacheMaxBytes), c.CacheMaxBytes, errMustNotBeNegative) } + err = c.validateUpstreamMaxResponseSize() + if err != nil { + return err + } + for _, host := range c.AllowlistHosts { err := validateAllowlistHost(host) if err != nil { @@ -556,6 +605,82 @@ func (c *Config) validate() error { errMustBeSetTogether) } + return c.validateAccessControlAllowOrigin() +} + +// validateUpstreamMaxResponseSize checks that upstream_max_response_size +// is from 1 byte to 1 GiB. An upstream response is read whole into +// memory, and the image processor reads one byte past this limit, which +// must not overflow. +func (c *Config) validateUpstreamMaxResponseSize() error { + const maxUpstreamMaxResponseSize = 1 << 30 // 1 GiB + if c.UpstreamMaxResponseSize < 1 || + c.UpstreamMaxResponseSize > maxUpstreamMaxResponseSize { + return fmt.Errorf("%s: value %d is %w 1-%d", + settingName(keyUpstreamMaxResponseSize), c.UpstreamMaxResponseSize, + errSizeOutOfRange, maxUpstreamMaxResponseSize) + } + + return nil +} + +// validateAccessControlAllowOrigin accepts "*" or an origin exactly as a browser +// sends it: http or https, an IP address as netip writes it or a lowercase name +// with a letter in its last part, and an optional port 1-65535, not the default. +func (c *Config) validateAccessControlAllowOrigin() error { + origin := c.AccessControlAllowOrigin + if origin == "*" { + return nil + } + + errOrigin := fmt.Errorf("%s: value %q is %w", + settingName(keyAccessControlAllowOrigin), origin, errNotAnOrigin) + + parsed, err := url.Parse(origin) + if err != nil { + return errOrigin + } + + defaultPort := map[string]string{"http": "80", "https": "443"}[parsed.Scheme] + if defaultPort == "" { + return errOrigin + } + + const letters = "abcdefghijklmnopqrstuvwxyz" + + host := parsed.Hostname() + lastPart := host[strings.LastIndex(host, ".")+1:] + + addr, err := netip.ParseAddr(host) + + switch { + case err == nil && addr.Is6(): + host = "[" + addr.String() + "]" + case err == nil: + host = addr.String() + case strings.Trim(host, letters+"0123456789-.") != "": // a character other than these + return errOrigin + case !strings.ContainsAny(lastPart, letters): + return errOrigin + } + + // The value must be exactly the origin rebuilt from its parts. + rebuilt := parsed.Scheme + "://" + host + + port := parsed.Port() + if port != "" { + _, err := strconv.ParseUint(port, 10, 16) + if err != nil || port[0] == '0' || port == defaultPort { + return errOrigin + } + + rebuilt += ":" + port + } + + if rebuilt != origin { + return errOrigin + } + return nil } @@ -673,6 +798,19 @@ func (l *strictLoader) int64Val(key string, defaultVal int64) int64 { return val } +func (l *strictLoader) durationVal(key string, defaultVal time.Duration) time.Duration { + if l.err != nil { + return 0 + } + + val, err := getDuration(l.sc, key, defaultVal) + if err != nil { + l.err = err + } + + return val +} + func (l *strictLoader) boolVal(key string, defaultVal bool) bool { if l.err != nil { return false @@ -794,6 +932,42 @@ func getInt64(sc *smartconfig.Config, key string, defaultVal int64) (int64, erro } } +// getDuration returns the duration value for key, or defaultVal if the +// key is omitted. A present value must be a positive Go duration string +// such as "30s" or "2m", read with time.ParseDuration; a bare number has +// no unit and is an error, as is an explicit null. +func getDuration( + sc *smartconfig.Config, key string, defaultVal time.Duration, +) (time.Duration, error) { + raw, ok := lookupValue(sc, key) + if !ok { + return defaultVal, nil + } + + if raw == nil { + return 0, errNullConfigValue(key) + } + + str, ok := raw.(string) + if !ok { + return 0, fmt.Errorf("config key %q: value %v (%T) is %w", + key, raw, raw, errNotADuration) + } + + parsed, err := time.ParseDuration(strings.TrimSpace(str)) + if err != nil { + return 0, fmt.Errorf("%s: value %q is %w", + settingName(key), str, errNotADuration) + } + + if parsed <= 0 { + return 0, fmt.Errorf("%s: value %q %w", + settingName(key), str, errMustBePositive) + } + + return parsed, nil +} + // getBool returns the boolean value for key, or defaultVal if the key // is omitted. A present value that is not a boolean (or a ParseBool-able // string), or is explicitly null, is an error; numbers are not accepted diff --git a/internal/config/config_validation_internal_test.go b/internal/config/config_validation_internal_test.go index c12d27d..c269a38 100644 --- a/internal/config/config_validation_internal_test.go +++ b/internal/config/config_validation_internal_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" "git.eeqj.de/sneak/smartconfig" ) @@ -599,3 +600,244 @@ func TestEnsureStateDirFailsOnUncreatablePath(t *testing.T) { t.Errorf("error %q does not name the offending key state_dir", err.Error()) } } + +// TestOmittedOriginTimeoutsAndSizeUseDefaults checks that the CORS +// origin, the upstream fetch timeout, the upstream response size limit +// and the downstream timeout default to the values pixa used before they +// could be configured. +func TestOmittedOriginTimeoutsAndSizeUseDefaults(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine) + if err != nil { + t.Fatalf("minimal config should be valid, got error: %v", err) + } + + if c.AccessControlAllowOrigin != "*" { + t.Errorf("AccessControlAllowOrigin = %q, want *", c.AccessControlAllowOrigin) + } + + if c.UpstreamFetchTimeout != 30*time.Second { + t.Errorf("UpstreamFetchTimeout = %v, want 30s", c.UpstreamFetchTimeout) + } + + if c.UpstreamMaxResponseSize != 50<<20 { + t.Errorf("UpstreamMaxResponseSize = %d, want %d (50 MiB)", + c.UpstreamMaxResponseSize, 50<<20) + } + + if c.DownstreamTimeout != 60*time.Second { + t.Errorf("DownstreamTimeout = %v, want 60s", c.DownstreamTimeout) + } +} + +// TestExplicitOriginTimeoutsAndSizeAreUsed checks that valid values for +// the CORS origin, the two timeouts and the response size limit are used +// as given. The size is the largest accepted, 1 GiB. +func TestExplicitOriginTimeoutsAndSizeAreUsed(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine+` +access_control_allow_origin: https://app.example.com +upstream_fetch_timeout: 10s +upstream_max_response_size: 1073741824 +downstream_timeout: 2m +`) + if err != nil { + t.Fatalf("valid config should load, got error: %v", err) + } + + if c.AccessControlAllowOrigin != "https://app.example.com" { + t.Errorf("AccessControlAllowOrigin = %q, want https://app.example.com", + c.AccessControlAllowOrigin) + } + + if c.UpstreamFetchTimeout != 10*time.Second { + t.Errorf("UpstreamFetchTimeout = %v, want 10s", c.UpstreamFetchTimeout) + } + + if c.UpstreamMaxResponseSize != 1073741824 { + t.Errorf("UpstreamMaxResponseSize = %d, want 1073741824", + c.UpstreamMaxResponseSize) + } + + if c.DownstreamTimeout != 2*time.Minute { + t.Errorf("DownstreamTimeout = %v, want 2m", c.DownstreamTimeout) + } +} + +// TestOriginWithPortOrAnyOriginIsAccepted checks the other accepted forms +// of access_control_allow_origin: "*", an origin with a port, and origins +// whose host is an IPv4 or IPv6 address. +func TestOriginWithPortOrAnyOriginIsAccepted(t *testing.T) { + t.Parallel() + + for _, origin := range []string{ + "*", "http://localhost:3000", "http://192.0.2.1", "http://[2001:db8::1]:8080", + } { + c, err := configFromYAML(t, signingKeyLine+ + "access_control_allow_origin: \""+origin+"\"\n") + if err != nil { + t.Fatalf("origin %q should be accepted, got error: %v", origin, err) + } + + if c.AccessControlAllowOrigin != origin { + t.Errorf("AccessControlAllowOrigin = %q, want %q", + c.AccessControlAllowOrigin, origin) + } + } +} + +// invalidTimeoutCases are configs where upstream_fetch_timeout or +// downstream_timeout is not a positive Go duration string; each must +// abort startup naming the key and the value. +func invalidTimeoutCases() []abortCase { + return []abortCase{ + { + name: "upstream_fetch_timeout not a duration", + yaml: signingKeyLine + "upstream_fetch_timeout: soon\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, "soon"}, + }, + { + name: "upstream_fetch_timeout number without a unit", + yaml: signingKeyLine + "upstream_fetch_timeout: 45\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, "45"}, + }, + { + name: "upstream_fetch_timeout zero", + yaml: signingKeyLine + "upstream_fetch_timeout: 0s\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, "0s"}, + }, + { + name: "upstream_fetch_timeout negative", + yaml: signingKeyLine + "upstream_fetch_timeout: -5s\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, "-5s"}, + }, + { + name: "upstream_fetch_timeout null", + yaml: signingKeyLine + "upstream_fetch_timeout: null\n", + wantErrSubstrings: []string{keyUpstreamFetchTimeout, nullValueText}, + }, + { + name: "downstream_timeout not a duration", + yaml: signingKeyLine + "downstream_timeout: 1 minute\n", + wantErrSubstrings: []string{keyDownstreamTimeout, "1 minute"}, + }, + { + name: "downstream_timeout zero", + yaml: signingKeyLine + "downstream_timeout: 0s\n", + wantErrSubstrings: []string{keyDownstreamTimeout, "0s"}, + }, + { + name: "downstream_timeout negative", + yaml: signingKeyLine + "downstream_timeout: -1m\n", + wantErrSubstrings: []string{keyDownstreamTimeout, "-1m"}, + }, + { + name: "downstream_timeout null", + yaml: signingKeyLine + "downstream_timeout:\n", + wantErrSubstrings: []string{keyDownstreamTimeout, nullValueText}, + }, + } +} + +// invalidSizeAndOriginCases are configs where upstream_max_response_size +// is not a whole number of bytes from 1 to 1 GiB, or +// access_control_allow_origin is neither "*" nor an origin; each must +// abort startup naming the key and the value. +func invalidSizeAndOriginCases() []abortCase { + badOrigins := []string{ + "", // empty + "example.com", // no scheme + "https://example.com/images", // a path + "https://example.com/", // a trailing slash + // The CORS middleware reads a * inside an origin as a pattern + // that lets other sites read responses. + "https://*", + "https://*.example.com", + "https://*example.com", + "https://a.com,b.com", // two hosts + "https://example.com:", // an empty port + "https://:8443", // no host + "https://example.com:0", // a port below 1 + "https://example.com:99999", // a port above 65535 + "https://exämple.com", // a host name that is not ASCII + "https://example.com:443", // the default port for https + "http://example.com:80", // the default port for http + "https://example.com:08080", // a port with a leading zero + "https://01.2.3.4", // an IPv4 address with a leading zero + "https://10.0.0", // an IPv4 address with three parts + "https://192.168.1.256", // an IPv4 address part above 255 + "https://example.123", // a host name whose last part is a number + "https://[0:0:0:0:0:0:0:1]", // an IPv6 address not in its shortest form + "file://example.com", // a scheme other than http or https + "HTTPS://example.com", // a scheme in upper case + "https://Example.com", // a host name in upper case + } + + cases := make([]abortCase, 0, len(badOrigins)) + for _, origin := range badOrigins { + cases = append(cases, abortCase{ + name: "access_control_allow_origin " + origin, + yaml: signingKeyLine + + "access_control_allow_origin: \"" + origin + "\"\n", + wantErrSubstrings: []string{keyAccessControlAllowOrigin, origin}, + }) + } + + return append(cases, []abortCase{ + { + name: "access_control_allow_origin null", + yaml: signingKeyLine + "access_control_allow_origin: null\n", + wantErrSubstrings: []string{keyAccessControlAllowOrigin, nullValueText}, + }, + { + name: "upstream_max_response_size with a unit", + yaml: signingKeyLine + "upstream_max_response_size: 50MB\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "50MB"}, + }, + { + name: "upstream_max_response_size fractional", + yaml: signingKeyLine + "upstream_max_response_size: 1.5\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "1.5"}, + }, + { + name: "upstream_max_response_size zero", + yaml: signingKeyLine + "upstream_max_response_size: 0\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "0"}, + }, + { + name: "upstream_max_response_size negative", + yaml: signingKeyLine + "upstream_max_response_size: -1\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "-1"}, + }, + { + name: "upstream_max_response_size null", + yaml: signingKeyLine + "upstream_max_response_size: null\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, nullValueText}, + }, + { + name: "upstream_max_response_size above 1 GiB", + yaml: signingKeyLine + "upstream_max_response_size: 1073741825\n", + wantErrSubstrings: []string{keyUpstreamMaxResponseSize, "1073741825"}, + }, + { + name: "upstream_max_response_size largest 64-bit integer", + yaml: signingKeyLine + + "upstream_max_response_size: 9223372036854775807\n", + wantErrSubstrings: []string{ + keyUpstreamMaxResponseSize, "9223372036854775807", + }, + }, + }...) +} + +// TestInvalidOriginTimeoutOrSizeAbortsStartup verifies the +// no-silent-fallback rule for the CORS origin, the two timeouts and the +// response size limit: a value that does not parse or is out of range +// aborts startup naming the key and the value. +func TestInvalidOriginTimeoutOrSizeAbortsStartup(t *testing.T) { + t.Parallel() + + runAbortCases(t, append(invalidTimeoutCases(), invalidSizeAndOriginCases()...)) +} diff --git a/internal/config/env_internal_test.go b/internal/config/env_internal_test.go index 57f1f15..cdc8790 100644 --- a/internal/config/env_internal_test.go +++ b/internal/config/env_internal_test.go @@ -8,6 +8,7 @@ import ( "slices" "strings" "testing" + "time" "sneak.berlin/go/pixa/internal/globals" "sneak.berlin/go/pixa/internal/logger" @@ -69,6 +70,10 @@ func TestEnvironmentSetsEveryKey(t *testing.T) { t.Setenv("PIXA_CACHE_MAX_BYTES", "1024") t.Setenv("PIXA_BLOCKED_NETWORKS", "203.0.113.0/24") t.Setenv("PIXA_TRUSTED_PROXIES", "192.0.2.0/24") + t.Setenv("PIXA_ACCESS_CONTROL_ALLOW_ORIGIN", "https://app.example.com") + t.Setenv("PIXA_UPSTREAM_FETCH_TIMEOUT", "10s") + t.Setenv("PIXA_UPSTREAM_MAX_RESPONSE_SIZE", "1048576") + t.Setenv("PIXA_DOWNSTREAM_TIMEOUT", "2m") c, err := newFromSmartConfig(nil) if err != nil { @@ -92,6 +97,10 @@ func TestEnvironmentSetsEveryKey(t *testing.T) { cacheMaxBytesExplicit: true, BlockedNetworks: []netip.Prefix{netip.MustParsePrefix("203.0.113.0/24")}, TrustedProxies: []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")}, + AccessControlAllowOrigin: "https://app.example.com", + UpstreamFetchTimeout: 10 * time.Second, + UpstreamMaxResponseSize: 1048576, + DownstreamTimeout: 2 * time.Minute, } if !reflect.DeepEqual(*c, want) { @@ -280,6 +289,30 @@ func TestInvalidDebugFromEnvironmentAbortsStartup(t *testing.T) { wantStartupError(t, err, "PIXA_DEBUG", "maybe") } +// TestInvalidOriginTimeoutOrSizeFromEnvironmentAbortsStartup checks that +// an invalid CORS origin, timeout or response size limit in its variable +// aborts startup naming the variable and the value. +func TestInvalidOriginTimeoutOrSizeFromEnvironmentAbortsStartup(t *testing.T) { + cases := []struct { + variable string + value string + }{ + {"PIXA_ACCESS_CONTROL_ALLOW_ORIGIN", "example.com"}, + {"PIXA_UPSTREAM_FETCH_TIMEOUT", "soon"}, + {"PIXA_UPSTREAM_MAX_RESPONSE_SIZE", "50MB"}, + {"PIXA_DOWNSTREAM_TIMEOUT", "0s"}, + } + + for _, tc := range cases { + t.Run(tc.variable, func(t *testing.T) { + t.Setenv(tc.variable, tc.value) + + _, err := configFromYAML(t, signingKeyLine) + wantStartupError(t, err, tc.variable, tc.value) + }) + } +} + // TestConfigFileAloneBehavesAsBefore checks that with no variables set // (TestMain unsets them) the config file's values are used and omitted // keys take their defaults. diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 09f3ffc..3da580d 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -106,6 +106,8 @@ func (s *Handlers) initImageService() error { // Create the fetcher config fetcherCfg := httpfetcher.DefaultConfig() fetcherCfg.AllowHTTP = s.config.AllowHTTP + fetcherCfg.Timeout = s.config.UpstreamFetchTimeout + fetcherCfg.MaxResponseSize = s.config.UpstreamMaxResponseSize if s.config.UpstreamConnectionsPerHost > 0 { fetcherCfg.MaxConnectionsPerHost = s.config.UpstreamConnectionsPerHost diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 30feaa5..492ba87 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -172,7 +172,7 @@ func (s *Middleware) Logging() func(http.Handler) http.Handler { // CORS returns a CORS middleware. func (s *Middleware) CORS() func(http.Handler) http.Handler { return cors.Handler(cors.Options{ - AllowedOrigins: []string{"*"}, + AllowedOrigins: []string{s.config.AccessControlAllowOrigin}, AllowedMethods: []string{"GET", "HEAD", "OPTIONS"}, AllowedHeaders: []string{"Accept", "Authorization", "Content-Type"}, ExposedHeaders: []string{"Link"}, diff --git a/internal/middleware/middleware_internal_test.go b/internal/middleware/middleware_internal_test.go index 9992a60..5dac1be 100644 --- a/internal/middleware/middleware_internal_test.go +++ b/internal/middleware/middleware_internal_test.go @@ -9,6 +9,53 @@ import ( "sneak.berlin/go/pixa/internal/config" ) +// TestCORSAnswersWithConfiguredOrigin checks that the CORS middleware +// answers with access_control_allow_origin, where "*" lets any origin read +// responses and a single origin lets only that origin read them. +func TestCORSAnswersWithConfiguredOrigin(t *testing.T) { + t.Parallel() + + const appOrigin = "https://app.example.com" + + cases := []struct { + configured string + requestOrigin string + want string + }{ + {"*", "https://any.example.com", "*"}, + {appOrigin, appOrigin, appOrigin}, + {appOrigin, "https://other.example.com", ""}, + } + + testHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + for _, tc := range cases { + mw := &Middleware{ + log: slog.Default(), + config: &config.Config{AccessControlAllowOrigin: tc.configured}, + } + + handler := mw.CORS()(testHandler) + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/v1/image/example.com/a.jpg/1x1.png", nil) + req.Header.Set("Origin", tc.requestOrigin) + + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + got := rec.Header().Get("Access-Control-Allow-Origin") + if got != tc.want { + t.Errorf("configured %q, request from %q: "+ + "Access-Control-Allow-Origin = %q, want %q", + tc.configured, tc.requestOrigin, got, tc.want) + } + } +} + func TestSecurityHeaders(t *testing.T) { t.Parallel() diff --git a/internal/server/http.go b/internal/server/http.go index bdd1238..13162e0 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -14,7 +14,6 @@ const ( // short, so a slowloris client dribbling headers is dropped well // before it ties up a connection for the whole ReadTimeout window. HTTPReadHeaderTimeout = 10 * time.Second - HTTPWriteTimeout = 60 * time.Second // HTTPIdleTimeout bounds how long an idle keep-alive connection is // held open, so idle connections cannot accumulate without limit on a // service targeting high concurrency. @@ -30,7 +29,7 @@ func (s *Server) newHTTPServer() *http.Server { Addr: fmt.Sprintf(":%d", s.config.Port), ReadTimeout: HTTPReadTimeout, ReadHeaderTimeout: HTTPReadHeaderTimeout, - WriteTimeout: HTTPWriteTimeout, + WriteTimeout: s.config.DownstreamTimeout, IdleTimeout: HTTPIdleTimeout, MaxHeaderBytes: HTTPMaxHeaderBytes, Handler: s, diff --git a/internal/server/http_internal_test.go b/internal/server/http_internal_test.go index d94a380..25b60fb 100644 --- a/internal/server/http_internal_test.go +++ b/internal/server/http_internal_test.go @@ -11,11 +11,15 @@ import ( // carries every hardening timeout wired onto it, including the slowloris // defense (ReadHeaderTimeout) and the keep-alive bound (IdleTimeout). This // guards against a field being defined but never set on the server, so -// each assertion compares the server field to its constant. +// each assertion compares the server field to its constant, or, for +// WriteTimeout, to downstream_timeout from the config. func TestNewHTTPServerTimeouts(t *testing.T) { t.Parallel() - s := &Server{config: &config.Config{Port: 8080}} + s := &Server{config: &config.Config{ + Port: 8080, + DownstreamTimeout: 45 * time.Second, + }} srv := s.newHTTPServer() @@ -26,7 +30,7 @@ func TestNewHTTPServerTimeouts(t *testing.T) { }{ {"ReadTimeout", srv.ReadTimeout, HTTPReadTimeout}, {"ReadHeaderTimeout", srv.ReadHeaderTimeout, HTTPReadHeaderTimeout}, - {"WriteTimeout", srv.WriteTimeout, HTTPWriteTimeout}, + {"WriteTimeout", srv.WriteTimeout, 45 * time.Second}, {"IdleTimeout", srv.IdleTimeout, HTTPIdleTimeout}, } diff --git a/internal/server/login_rate_limit_internal_test.go b/internal/server/login_rate_limit_internal_test.go index 13195c0..c37d86e 100644 --- a/internal/server/login_rate_limit_internal_test.go +++ b/internal/server/login_rate_limit_internal_test.go @@ -51,11 +51,12 @@ func newTestServer(t *testing.T) *Server { stateDir := t.TempDir() cfg := &config.Config{ - Debug: true, - SigningKey: testSigningKey, - StateDir: stateDir, - DBURL: "file:" + filepath.Join(stateDir, "state.sqlite3"), - TrustedProxies: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}, + Debug: true, + SigningKey: testSigningKey, + StateDir: stateDir, + DBURL: "file:" + filepath.Join(stateDir, "state.sqlite3"), + TrustedProxies: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}, + DownstreamTimeout: config.DefaultDownstreamTimeout, } lc := fxtest.NewLifecycle(t) diff --git a/internal/server/routes.go b/internal/server/routes.go index 0bb2748..4623b75 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -33,7 +33,7 @@ func (s *Server) SetupRoutes() { } s.router.Use(s.mw.CORS()) - s.router.Use(middleware.Timeout(HTTPWriteTimeout)) + s.router.Use(middleware.Timeout(s.config.DownstreamTimeout)) if s.sentryEnabled { sentryHandler := sentryhttp.New(sentryhttp.Options{ -- 2.54.0 From 0c99be939a1072602226e8932f6502558bf42ace Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 09:11:20 +0200 Subject: [PATCH 07/11] Install Dockerfile build dependencies through script/bootstrap (closes #95) The Dockerfile lint and build stages and Dockerfile.lint each carried their own apk add list, a copy of what script/bootstrap installs. They now copy script/, go.mod and go.sum and run script/bootstrap, so that layer is reused until one of those changes. script/bootstrap gains a C compiler check: the golang image has none, and cgo needs one. The build adds -trimpath and -s -w; CGO_ENABLED=1 stays, as govips links libvips. ARG VERSION moves to just above the build, so a new version reruns neither script/bootstrap nor the tests. Model: opus-5-5 --- Dockerfile | 33 ++++++++++++++++----------------- Dockerfile.lint | 14 +++++++------- TODO.md | 8 ++++++++ script/bootstrap | 11 +++++++++-- 4 files changed, 40 insertions(+), 26 deletions(-) diff --git a/Dockerfile b/Dockerfile index 45027be..76d9bac 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,13 +3,14 @@ # golangci/golangci-lint:v2.12.2-alpine, 2026-08-07 FROM golangci/golangci-lint:v2.12.2-alpine@sha256:91b27804074a0bacea298707f016911e60cf0cdbc6c7bf5ccacb5f0606d18d60 AS lint -RUN apk add --no-cache make build-base vips-dev libheif-dev pkgconfig - WORKDIR /src -# Copy go mod files first for better layer caching +# script/bootstrap installs the build dependencies and downloads the Go +# modules. Only script/, go.mod and go.sum are copied first, so this +# layer is reused until one of them changes. +COPY script/ ./script/ COPY go.mod go.sum ./ -RUN go mod download +RUN script/bootstrap # Copy source code COPY . . @@ -28,20 +29,12 @@ FROM golang:1.25.4-alpine@sha256:d3f0cf7723f3429e3f9ed846243970b20a2de7bae6a5b66 # Depend on lint stage passing COPY --from=lint /src/go.sum /dev/null -ARG VERSION=dev - -# Install build dependencies for CGO image libraries -RUN apk add --no-cache \ - build-base \ - vips-dev \ - libheif-dev \ - pkgconfig - WORKDIR /src -# Copy go mod files first for better layer caching +# Build dependencies and Go modules, as in the lint stage +COPY script/ ./script/ COPY go.mod go.sum ./ -RUN GOTOOLCHAIN=auto go mod download +RUN script/bootstrap # Copy source code COPY . . @@ -49,8 +42,14 @@ COPY . . # Run tests RUN make test -# Build with CGO enabled -RUN CGO_ENABLED=1 GOTOOLCHAIN=auto go build -ldflags "-X main.Version=${VERSION}" -o /pixad ./cmd/pixad +# VERSION is declared here, not earlier: a new value reruns only the +# build, not script/bootstrap or the tests. CGO stays enabled for +# govips; -trimpath keeps build paths out of the binary, and -s -w +# leave out the symbol table and debug information. +ARG VERSION=dev +RUN CGO_ENABLED=1 GOTOOLCHAIN=auto go build -trimpath \ + -ldflags "-s -w -X main.Version=${VERSION}" \ + -o /pixad ./cmd/pixad # Runtime stage # alpine:3.21, 2026-02-25 diff --git a/Dockerfile.lint b/Dockerfile.lint index a07d8ca..41e350b 100644 --- a/Dockerfile.lint +++ b/Dockerfile.lint @@ -6,16 +6,16 @@ # golangci/golangci-lint:v2.12.2-alpine, 2026-08-07 FROM golangci/golangci-lint:v2.12.2-alpine@sha256:91b27804074a0bacea298707f016911e60cf0cdbc6c7bf5ccacb5f0606d18d60 -# pixa is CGO/libvips: the type-aware linters compile every package, so -# this image needs the same C libraries the build does. -RUN apk add --no-cache build-base vips-dev libheif-dev pkgconfig - WORKDIR /src -# Modules first for layer caching; go.mod/go.sum settle this layer's -# result, so it may safely be reused between runs. +# pixa is CGO/libvips: the type-aware linters compile every package, so +# this image needs the same C libraries the build does. script/bootstrap +# installs them and downloads the Go modules. Only script/, go.mod and +# go.sum are copied first; they settle this layer's result, so it may +# safely be reused between runs. +COPY script/ ./script/ COPY go.mod go.sum ./ -RUN go mod download +RUN script/bootstrap COPY . . diff --git a/TODO.md b/TODO.md index e6ad439..c252259 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,14 @@ exhaustion # Completed Steps +- 2026-09-29 Dockerfiles install through `script/bootstrap` (closes #95): the + `Dockerfile` lint and build stages and `Dockerfile.lint` copy `script/`, + `go.mod` and `go.sum`, then run `script/bootstrap` in place of their own + `apk add` lines, so the build dependencies are listed in one place; + `script/bootstrap` now also installs a C compiler when `gcc` is missing; the + build uses `-trimpath` and `-s -w` and keeps `CGO_ENABLED=1` for govips; + `ARG VERSION` sits just above the build, so a new version reruns neither + `script/bootstrap` nor the tests. - 2026-09-29 migrations at the path `REPO_POLICIES.md` sets (closes #96): the migration files moved, contents unchanged, from `internal/database/schema/` to `internal/db/migrations/` as `000_migration.sql` and `001_schema.sql`; the diff --git a/script/bootstrap b/script/bootstrap index 83d2fc5..54c04ea 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -5,8 +5,10 @@ # or apk (detected in that order); assumes NOTHING is present (not git, # make, or go). The linter is never installed on the host: golangci-lint # runs only inside a container, Dockerfile.lint or the Dockerfile lint -# stage (see script/lint). CGO image libraries (pkg-config, vips, -# libheif) are installed for the govips bindings. +# stage (see script/lint). A C compiler and the CGO image libraries +# (pkg-config, vips, libheif) are installed for the govips bindings. +# Both Dockerfiles run this script too, so their build dependencies are +# the ones listed here. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" @@ -53,6 +55,11 @@ missing() { # CGO dependencies for govips (image processing) ensure_cgo_deps() { + # cgo compiles with gcc on Linux; build-base and build-essential + # also bring the C library headers. + if missing gcc; then + pkg_install gcc build-essential gcc build-base + fi if missing pkg-config; then pkg_install pkg-config pkg-config pkg-config pkgconfig fi -- 2.54.0 From baf457d21e928596af08b804ed5f38eb95587dc8 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 10:32:16 +0200 Subject: [PATCH 08/11] Bound concurrent image processing and upstream fetches (closes #64) Nothing bounded total in-flight work, so a burst of cache misses across hosts could exhaust memory. Two settings now do: max_concurrent_processing (default the CPUs Go uses) and upstream_connections (default 64, beside the per-host limit). A request that finds either full waits up to 10 seconds, then gets 503; a slot is released on every path. No request holds source bytes while it waits: a cached source is read only after the processing slot is taken, and a fetched one only while it holds its upstream connection. libvips runs one worker thread per image with its operation cache off. Both waits count toward downstream_timeout, as the README says. Model: opus-5-5 --- README.md | 18 +- TODO.md | 14 +- config.example.yml | 19 +- .../concurrency_limits_internal_test.go | 162 ++++++++++ internal/config/config.go | 71 +++- internal/config/env_internal_test.go | 4 + internal/handlers/handlers.go | 12 +- internal/handlers/image.go | 9 + internal/handlers/imageenc.go | 5 + .../handlers/server_busy_internal_test.go | 49 +++ internal/httpfetcher/httpfetcher.go | 105 ++++-- .../max_connections_internal_test.go | 165 ++++++++++ internal/imageprocessor/imageprocessor.go | 79 ++++- ...max_concurrent_processing_internal_test.go | 302 ++++++++++++++++++ internal/imgcache/cache.go | 11 +- ...max_concurrent_processing_internal_test.go | 125 ++++++++ internal/imgcache/service.go | 81 +++-- 17 files changed, 1137 insertions(+), 94 deletions(-) create mode 100644 internal/config/concurrency_limits_internal_test.go create mode 100644 internal/handlers/server_busy_internal_test.go create mode 100644 internal/httpfetcher/max_connections_internal_test.go create mode 100644 internal/imageprocessor/max_concurrent_processing_internal_test.go create mode 100644 internal/imgcache/max_concurrent_processing_internal_test.go diff --git a/README.md b/README.md index 7a3a013..001d863 100644 --- a/README.md +++ b/README.md @@ -235,6 +235,8 @@ variables set by the file's `env:` section are checked the same way. | `PIXA_TRUSTED_PROXIES` | `trusted_proxies` | CIDR ranges of proxies whose `X-Forwarded-For` is believed; default RFC 1918 | | `PIXA_ALLOW_HTTP` | `allow_http` | Allow plain-HTTP upstreams, for testing only; default `false` | | `PIXA_UPSTREAM_CONNECTIONS_PER_HOST` | `upstream_connections_per_host` | Concurrent connections per upstream host; default `20` | +| `PIXA_UPSTREAM_CONNECTIONS` | `upstream_connections` | Concurrent connections to all upstream hosts together; default `64` | +| `PIXA_MAX_CONCURRENT_PROCESSING` | `max_concurrent_processing` | Images processed at once; default the number of CPUs | | `PIXA_UPSTREAM_FETCH_TIMEOUT` | `upstream_fetch_timeout` | Time allowed for one fetch from an upstream host; default `30s` | | `PIXA_UPSTREAM_MAX_RESPONSE_SIZE` | `upstream_max_response_size` | Largest upstream response accepted, in bytes; default 50 MiB | | `PIXA_DOWNSTREAM_TIMEOUT` | `downstream_timeout` | Time allowed for answering one client request; default `60s` | @@ -286,12 +288,24 @@ Key settings in more detail: bytes; default `52428800` (50 MiB). It also limits the image data pixa decodes - `downstream_timeout` — time allowed for answering one client request, as a - duration; default `60s`. The upstream fetch counts toward it, so keep it - longer than `upstream_fetch_timeout` + duration; default `60s`. The upstream fetch counts toward it, and so do the + waits for an upstream connection and for a processing slot (up to 10 seconds + each), so keep it longer than `upstream_fetch_timeout` plus 20 seconds - `signing_key` — HMAC secret for URL signatures - `cache_max_bytes` — disk cache size limit in bytes; `0` disables the disk cache entirely; omitted defaults to 75% of the free space on the filesystem containing `/cache/` (minimum 500 MiB) +- `upstream_connections` — the most connections to upstream hosts at once, all + hosts together, on top of `upstream_connections_per_host`; default `64`. A + fetch holds its connection until its image has been processed. A fetch that + finds all of them in use waits up to 10 seconds for one to free up; if none + does, and `downstream_timeout` has not ended first, the request is answered + 503 with the error `server busy, try again later` +- `max_concurrent_processing` — the most images decoded and encoded at once; + default the number of CPUs pixa can use (`GOMAXPROCS`), which follows a + container's CPU limit. A request that finds all of them in use waits up to 10 + seconds for one to free up; if none does, and `downstream_timeout` has not + ended first, it is answered 503 the same way See `config.example.yml` for all options with defaults. diff --git a/TODO.md b/TODO.md index c252259..8276df8 100644 --- a/TODO.md +++ b/TODO.md @@ -25,11 +25,20 @@ The disk cache is now size-bounded with LRU eviction # Next Step -P1: rate limit global concurrent upstream fetches to prevent resource -exhaustion +P2: security: referer blacklist # Completed Steps +- 2026-09-29 bound concurrent image processing and upstream fetches (closes + #64): `max_concurrent_processing` (default the number of CPUs pixa can use) + limits the images decoded and encoded at once, and `upstream_connections` + (default 64) the connections to all upstream hosts together, on top of + `upstream_connections_per_host`; a fetch holds its connection until its image + has been processed, and a request whose source is cached reads it only once it + has a processing slot; a request that finds either limit reached waits up to + 10 seconds for a free one, then gets 503 `server busy, try again later`; + libvips runs one worker thread per image with its operation cache off; + documented in `README.md` and `config.example.yml`. - 2026-09-29 Dockerfiles install through `script/bootstrap` (closes #95): the `Dockerfile` lint and build stages and `Dockerfile.lint` copy `script/`, `go.mod` and `go.sum`, then run `script/bootstrap` in place of their own @@ -316,7 +325,6 @@ exhaustion # Future Steps - P2: security - - referer blacklist - per-IP rate limiting on the image routes - per-origin rate limiting - P2: HTTP response handling diff --git a/config.example.yml b/config.example.yml index 27c2422..d64d77e 100644 --- a/config.example.yml +++ b/config.example.yml @@ -71,6 +71,19 @@ allow_http: false # Maximum concurrent connections per upstream host (default: 20) upstream_connections_per_host: 20 +# Maximum concurrent connections to all upstream hosts together, on top of +# the per-host limit (default: 64). A fetch holds its connection until its +# image has been processed. A fetch that finds none free waits up to 10 +# seconds for one, and if none frees up the request is answered 503, unless +# downstream_timeout has ended first. +upstream_connections: 64 + +# Maximum number of images decoded and encoded at once (default: the +# number of CPUs pixa can use, which follows a container's CPU limit). A +# request that finds none free waits up to 10 seconds for one, and if none +# frees up it is answered 503, unless downstream_timeout has ended first. +# max_concurrent_processing: 4 + # Time allowed for one fetch from an upstream host (default: 30s) upstream_fetch_timeout: 30s @@ -78,8 +91,10 @@ upstream_fetch_timeout: 30s # (1 GiB) (default: 52428800, 50 MiB) upstream_max_response_size: 52428800 -# Time allowed for answering one client request, the upstream fetch -# included, so keep it longer than upstream_fetch_timeout (default: 60s) +# Time allowed for answering one client request (default: 60s). The +# upstream fetch counts toward it, and so do the waits for an upstream +# connection and for a processing slot (up to 10 seconds each), so keep it +# longer than upstream_fetch_timeout plus 20 seconds. downstream_timeout: 60s # The origin a browser lets read pixa's responses, sent as the CORS diff --git a/internal/config/concurrency_limits_internal_test.go b/internal/config/concurrency_limits_internal_test.go new file mode 100644 index 0000000..c5f4b20 --- /dev/null +++ b/internal/config/concurrency_limits_internal_test.go @@ -0,0 +1,162 @@ +package config + +import ( + "runtime" + "testing" +) + +// The variables that set the two concurrency limits. +const ( + testMaxConcurrentProcessingVar = "PIXA_MAX_CONCURRENT_PROCESSING" + testUpstreamConnectionsVar = "PIXA_UPSTREAM_CONNECTIONS" +) + +// TestOmittedConcurrencyLimitsUseDefaults checks that an omitted +// max_concurrent_processing is the number of CPUs Go uses and an omitted +// upstream_connections is 64. +func TestOmittedConcurrencyLimitsUseDefaults(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine) + if err != nil { + t.Fatalf("minimal config should be valid, got error: %v", err) + } + + if c.MaxConcurrentProcessing != runtime.GOMAXPROCS(0) { + t.Errorf("MaxConcurrentProcessing = %d, want %d, one per CPU", + c.MaxConcurrentProcessing, runtime.GOMAXPROCS(0)) + } + + if c.UpstreamConnections != 64 { + t.Errorf("UpstreamConnections = %d, want 64", c.UpstreamConnections) + } +} + +// TestExplicitConcurrencyLimitsAreUsed checks that valid values for the +// two limits are used as given. +func TestExplicitConcurrencyLimitsAreUsed(t *testing.T) { + t.Parallel() + + c, err := configFromYAML(t, signingKeyLine+ + "max_concurrent_processing: 3\nupstream_connections: 10\n") + if err != nil { + t.Fatalf("valid config should load, got error: %v", err) + } + + if c.MaxConcurrentProcessing != 3 { + t.Errorf("MaxConcurrentProcessing = %d, want 3", c.MaxConcurrentProcessing) + } + + if c.UpstreamConnections != 10 { + t.Errorf("UpstreamConnections = %d, want 10", c.UpstreamConnections) + } +} + +// TestInvalidConcurrencyLimitAbortsStartup checks that a limit that is +// not a whole number of at least 1, or is null, aborts startup naming the +// key and the value, and the variable too where the value could have come +// from it. +func TestInvalidConcurrencyLimitAbortsStartup(t *testing.T) { + t.Parallel() + + processing := keyMaxConcurrentProcessing + connections := keyUpstreamConnections + + runAbortCases(t, []abortCase{ + { + name: "max_concurrent_processing zero", + yaml: signingKeyLine + processing + ": 0\n", + wantErrSubstrings: []string{ + processing, testMaxConcurrentProcessingVar, "value 0", + }, + }, + { + name: "max_concurrent_processing negative", + yaml: signingKeyLine + processing + ": -2\n", + wantErrSubstrings: []string{ + processing, testMaxConcurrentProcessingVar, "value -2", + }, + }, + { + name: "max_concurrent_processing not a number", + yaml: signingKeyLine + processing + ": lots\n", + wantErrSubstrings: []string{ + processing, testMaxConcurrentProcessingVar, "lots", + }, + }, + { + name: "max_concurrent_processing fractional", + yaml: signingKeyLine + processing + ": 1.5\n", + wantErrSubstrings: []string{processing, "1.5"}, + }, + { + name: "max_concurrent_processing null", + yaml: signingKeyLine + processing + ": null\n", + wantErrSubstrings: []string{processing, nullValueText}, + }, + { + name: "upstream_connections zero", + yaml: signingKeyLine + connections + ": 0\n", + wantErrSubstrings: []string{ + connections, testUpstreamConnectionsVar, "value 0", + }, + }, + { + name: "upstream_connections negative", + yaml: signingKeyLine + connections + ": -5\n", + wantErrSubstrings: []string{ + connections, testUpstreamConnectionsVar, "value -5", + }, + }, + { + name: "upstream_connections not a number", + yaml: signingKeyLine + connections + ": many\n", + wantErrSubstrings: []string{ + connections, testUpstreamConnectionsVar, "many", + }, + }, + { + name: "upstream_connections null", + yaml: signingKeyLine + connections + ": null\n", + wantErrSubstrings: []string{connections, nullValueText}, + }, + }) +} + +// TestConcurrencyLimitsFromEnvironment checks that the two variables set +// the limits over the config file, and that an invalid value in either +// aborts startup naming the variable and the value. +func TestConcurrencyLimitsFromEnvironment(t *testing.T) { + t.Setenv(testMaxConcurrentProcessingVar, "3") + t.Setenv(testUpstreamConnectionsVar, "10") + + c, err := configFromYAML(t, signingKeyLine+ + "max_concurrent_processing: 5\nupstream_connections: 50\n") + if err != nil { + t.Fatalf("limits from the environment should load: %v", err) + } + + if c.MaxConcurrentProcessing != 3 || c.UpstreamConnections != 10 { + t.Errorf("limits = %d and %d, want 3 and 10 from the environment", + c.MaxConcurrentProcessing, c.UpstreamConnections) + } + + cases := []struct { + variable string + value string + }{ + {testMaxConcurrentProcessingVar, "lots"}, + {testMaxConcurrentProcessingVar, "0"}, + {testUpstreamConnectionsVar, "-1"}, + {testUpstreamConnectionsVar, "ten"}, + } + + for _, tc := range cases { + t.Run(tc.variable+"="+tc.value, func(t *testing.T) { + t.Setenv(tc.variable, tc.value) + + _, err := configFromYAML(t, signingKeyLine) + wantStartupError(t, err, tc.variable, tc.value) + }) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 5c9504f..b8e041b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -10,6 +10,7 @@ import ( "net/url" "os" "path/filepath" + "runtime" "sort" "strconv" "strings" @@ -25,6 +26,7 @@ const ( DefaultPort = 8080 DefaultStateDir = "/var/lib/pixa" DefaultUpstreamConnectionsPerHost = 20 + DefaultUpstreamConnections = 64 DefaultAccessControlAllowOrigin = "*" DefaultUpstreamFetchTimeout = 30 * time.Second DefaultUpstreamMaxResponseSize = 50 << 20 // 50 MiB @@ -46,6 +48,8 @@ const ( keyAllowlistHosts = "allowlist_hosts" keyAllowHTTP = "allow_http" keyUpstreamConnectionsPerHost = "upstream_connections_per_host" + keyUpstreamConnections = "upstream_connections" + keyMaxConcurrentProcessing = "max_concurrent_processing" keyCacheMaxBytes = "cache_max_bytes" keyBlockedNetworks = "blocked_networks" keyTrustedProxies = "trusted_proxies" @@ -79,7 +83,7 @@ var ( errNotAValidURL = errors.New("not a valid URL") errPortOutOfRange = errors.New("outside the valid port range") errSizeOutOfRange = errors.New("outside the accepted range") - errTooFewConnections = errors.New("must be at least 1") + errMustBeAtLeastOne = errors.New("must be at least 1") errValueTooShort = errors.New("value too short") errPlaceholderKey = errors.New( "is the placeholder from config.example.yml; " + @@ -126,6 +130,12 @@ type Config struct { AllowHTTP bool // Allow non-TLS upstream (testing only) UpstreamConnectionsPerHost int // Max concurrent connections per upstream host + // UpstreamConnections is the most concurrent connections to all + // upstream hosts together, on top of the per-host limit. + // MaxConcurrentProcessing is the most images processed at once. + UpstreamConnections int + MaxConcurrentProcessing int + // UpstreamFetchTimeout is the time allowed for one fetch from an // upstream host. UpstreamMaxResponseSize is the largest upstream // response accepted, in bytes, and also the image processor's input @@ -226,14 +236,12 @@ func New(_ fx.Lifecycle, params Params) (*Config, error) { // unparseable or invalid is an error: defaults apply only to omitted // keys, never to invalid explicit values. func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { - if sc != nil { - err := validateKnownKeys(sc) - if err != nil { - return nil, err - } + err := validateKnownKeys(sc) + if err != nil { + return nil, err } - err := validateAllowlistHostsValue(sc) + err = validateAllowlistHostsValue(sc) if err != nil { return nil, err } @@ -271,6 +279,12 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { AllowHTTP: loader.boolVal(keyAllowHTTP, false), UpstreamConnectionsPerHost: loader.intVal( keyUpstreamConnectionsPerHost, DefaultUpstreamConnectionsPerHost), + UpstreamConnections: loader.intVal( + keyUpstreamConnections, DefaultUpstreamConnections), + // Decoding and encoding are CPU-bound, so the default is one image + // per CPU Go uses, which follows a container's CPU limit. + MaxConcurrentProcessing: loader.intVal( + keyMaxConcurrentProcessing, runtime.GOMAXPROCS(0)), UpstreamFetchTimeout: loader.durationVal( keyUpstreamFetchTimeout, DefaultUpstreamFetchTimeout), UpstreamMaxResponseSize: loader.int64Val( @@ -321,8 +335,13 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) { // being silently ignored, and rejects keys that are explicitly set to // null: a null is a SET value, never an omission, so it must not // silently take the default. The env section is permitted because -// smartconfig consumes it for environment variable injection. +// smartconfig consumes it for environment variable injection. A nil sc +// means no config file, which has no keys to check. func validateKnownKeys(sc *smartconfig.Config) error { + if sc == nil { + return nil + } + var unknown, nullKeys []string for key, value := range sc.Data() { @@ -392,7 +411,8 @@ func isKnownConfigKey(key string) bool { switch key { case keyDebug, keyMaintenanceMode, keyPort, keyStateDir, keySentryDSN, keyDBURL, keyMetrics, keySigningKey, keyAllowlistHosts, keyAllowHTTP, - keyUpstreamConnectionsPerHost, keyCacheMaxBytes, keyBlockedNetworks, + keyUpstreamConnectionsPerHost, keyUpstreamConnections, + keyMaxConcurrentProcessing, keyCacheMaxBytes, keyBlockedNetworks, keyTrustedProxies, keyAccessControlAllowOrigin, keyUpstreamFetchTimeout, keyUpstreamMaxResponseSize, keyDownstreamTimeout, "env": return true @@ -419,6 +439,8 @@ func envVarNames() map[string]string { keyAllowlistHosts: "PIXA_ALLOWLIST_HOSTS", keyAllowHTTP: "PIXA_ALLOW_HTTP", keyUpstreamConnectionsPerHost: "PIXA_UPSTREAM_CONNECTIONS_PER_HOST", + keyUpstreamConnections: "PIXA_UPSTREAM_CONNECTIONS", + keyMaxConcurrentProcessing: "PIXA_MAX_CONCURRENT_PROCESSING", keyCacheMaxBytes: "PIXA_CACHE_MAX_BYTES", keyBlockedNetworks: "PIXA_BLOCKED_NETWORKS", keyTrustedProxies: "PIXA_TRUSTED_PROXIES", @@ -562,10 +584,9 @@ func (c *Config) validate() error { settingName(keyPort), c.Port, errPortOutOfRange, maxPort) } - if c.UpstreamConnectionsPerHost < 1 { - return fmt.Errorf("%s: value %d %w", - settingName(keyUpstreamConnectionsPerHost), - c.UpstreamConnectionsPerHost, errTooFewConnections) + err = c.validateConcurrencyLimits() + if err != nil { + return err } if c.StateDir == "" { @@ -684,6 +705,30 @@ func (c *Config) validateAccessControlAllowOrigin() error { return nil } +// validateConcurrencyLimits checks that the two upstream connection limits +// and the image processing limit are at least 1. +func (c *Config) validateConcurrencyLimits() error { + if c.UpstreamConnectionsPerHost < 1 { + return fmt.Errorf("%s: value %d %w", + settingName(keyUpstreamConnectionsPerHost), + c.UpstreamConnectionsPerHost, errMustBeAtLeastOne) + } + + if c.UpstreamConnections < 1 { + return fmt.Errorf("%s: value %d %w", + settingName(keyUpstreamConnections), + c.UpstreamConnections, errMustBeAtLeastOne) + } + + if c.MaxConcurrentProcessing < 1 { + return fmt.Errorf("%s: value %d %w", + settingName(keyMaxConcurrentProcessing), + c.MaxConcurrentProcessing, errMustBeAtLeastOne) + } + + return nil +} + // validateAllowlistHost checks that an allowlist_hosts entry is a bare // hostname, optionally with a leading dot for suffix matching. URLs, // paths, and whitespace indicate a misconfigured entry. An entry with diff --git a/internal/config/env_internal_test.go b/internal/config/env_internal_test.go index cdc8790..e61ffbf 100644 --- a/internal/config/env_internal_test.go +++ b/internal/config/env_internal_test.go @@ -67,6 +67,8 @@ func TestEnvironmentSetsEveryKey(t *testing.T) { t.Setenv("PIXA_ALLOWLIST_HOSTS", "s3.sneak.cloud,.example.com") t.Setenv("PIXA_ALLOW_HTTP", "true") t.Setenv("PIXA_UPSTREAM_CONNECTIONS_PER_HOST", "5") + t.Setenv("PIXA_UPSTREAM_CONNECTIONS", "10") + t.Setenv("PIXA_MAX_CONCURRENT_PROCESSING", "3") t.Setenv("PIXA_CACHE_MAX_BYTES", "1024") t.Setenv("PIXA_BLOCKED_NETWORKS", "203.0.113.0/24") t.Setenv("PIXA_TRUSTED_PROXIES", "192.0.2.0/24") @@ -93,6 +95,8 @@ func TestEnvironmentSetsEveryKey(t *testing.T) { AllowlistHosts: []string{testHostS3, ".example.com"}, AllowHTTP: true, UpstreamConnectionsPerHost: 5, + UpstreamConnections: 10, + MaxConcurrentProcessing: 3, CacheMaxBytes: 1024, cacheMaxBytesExplicit: true, BlockedNetworks: []netip.Prefix{netip.MustParsePrefix("203.0.113.0/24")}, diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 3da580d..b350595 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -113,15 +113,17 @@ func (s *Handlers) initImageService() error { fetcherCfg.MaxConnectionsPerHost = s.config.UpstreamConnectionsPerHost } + fetcherCfg.MaxConnections = s.config.UpstreamConnections fetcherCfg.BlockedNetworks = s.config.BlockedNetworks // Create the service svc, err := imgcache.NewService(&imgcache.ServiceConfig{ - Cache: cache, - FetcherConfig: fetcherCfg, - SigningKey: s.config.SigningKey, - Allowlist: s.config.AllowlistHosts, - Logger: s.log, + Cache: cache, + FetcherConfig: fetcherCfg, + SigningKey: s.config.SigningKey, + Allowlist: s.config.AllowlistHosts, + MaxConcurrentProcessing: s.config.MaxConcurrentProcessing, + Logger: s.log, }) if err != nil { return err diff --git a/internal/handlers/image.go b/internal/handlers/image.go index 561ca1d..3c94bc8 100644 --- a/internal/handlers/image.go +++ b/internal/handlers/image.go @@ -12,6 +12,7 @@ import ( "github.com/go-chi/chi/v5" "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" + "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imgcache" ) @@ -217,6 +218,14 @@ func (s *Handlers) respondImageError( return } + if errors.Is(err, httpfetcher.ErrTooManyConnections) || + errors.Is(err, imageprocessor.ErrTooManyImages) { + s.respondError(w, "server busy, try again later", + http.StatusServiceUnavailable) + + return + } + s.respondError(w, "internal error", http.StatusInternalServerError) } diff --git a/internal/handlers/imageenc.go b/internal/handlers/imageenc.go index 5effd8b..56af31b 100644 --- a/internal/handlers/imageenc.go +++ b/internal/handlers/imageenc.go @@ -12,6 +12,7 @@ import ( "sneak.berlin/go/pixa/internal/encurl" "sneak.berlin/go/pixa/internal/httpfetcher" + "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/imgcache" ) @@ -124,6 +125,10 @@ func (s *Handlers) handleImageError(w http.ResponseWriter, err error) { s.respondError(w, "upstream error", http.StatusBadGateway) case errors.Is(err, httpfetcher.ErrUpstreamTimeout): s.respondError(w, "upstream timeout", http.StatusGatewayTimeout) + case errors.Is(err, httpfetcher.ErrTooManyConnections), + errors.Is(err, imageprocessor.ErrTooManyImages): + s.respondError(w, "server busy, try again later", + http.StatusServiceUnavailable) default: s.log.Error("image request failed", "error", err) s.respondError(w, "internal error", http.StatusInternalServerError) diff --git a/internal/handlers/server_busy_internal_test.go b/internal/handlers/server_busy_internal_test.go new file mode 100644 index 0000000..2f3471c --- /dev/null +++ b/internal/handlers/server_busy_internal_test.go @@ -0,0 +1,49 @@ +package handlers + +import ( + "fmt" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "sneak.berlin/go/pixa/internal/httpfetcher" + "sneak.berlin/go/pixa/internal/imageprocessor" + "sneak.berlin/go/pixa/internal/imgcache" +) + +// TestServerBusyAnswers503 checks that both image routes answer 503 with +// a clear error when the image service gives up waiting for a free upstream +// connection or processing slot, wrapped as the service wraps them. +func TestServerBusyAnswers503(t *testing.T) { + t.Parallel() + + h := &Handlers{log: slog.New(slog.DiscardHandler)} + req := &imgcache.ImageRequest{SourceHost: "img.example.com", SourcePath: "/a.jpg"} + + for _, err := range []error{ + fmt.Errorf("upstream fetch failed: %w", httpfetcher.ErrTooManyConnections), + fmt.Errorf("image processing failed: %w", imageprocessor.ErrTooManyImages), + } { + plain := httptest.NewRecorder() + h.respondImageError(plain, req, err) + + encrypted := httptest.NewRecorder() + h.handleImageError(encrypted, err) + + for route, rec := range map[string]*httptest.ResponseRecorder{ + "/v1/image/": plain, "/v1/e/": encrypted, + } { + if rec.Code != http.StatusServiceUnavailable { + t.Errorf("%s for %v: status = %d, want %d", + route, err, rec.Code, http.StatusServiceUnavailable) + } + + if !strings.Contains(rec.Body.String(), "server busy, try again later") { + t.Errorf("%s for %v: body = %q, want the server busy error", + route, err, rec.Body.String()) + } + } + } +} diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index 64e27eb..d65f2da 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/internal/httpfetcher/httpfetcher.go @@ -1,5 +1,6 @@ // Package httpfetcher fetches content from upstream HTTP origins with SSRF -// protection, per-host connection limits, and content-type validation. +// protection, connection limits per host and for all hosts together, and +// content-type validation. package httpfetcher import ( @@ -28,8 +29,13 @@ const ( DefaultIdleConnTimeout = 90 * time.Second DefaultMaxRedirects = 10 DefaultMaxConnectionsPerHost = 20 + DefaultMaxConnections = 64 ) +// ConnectionWaitTimeout is how long Fetch waits for a free connection when +// MaxConnections fetches are already in progress. +const ConnectionWaitTimeout = 10 * time.Second + // MIME content types. const ( contentTypeJPEG = "image/jpeg" @@ -70,6 +76,7 @@ var ( ErrInvalidContentType = errors.New("invalid or unsupported content type") ErrUpstreamError = errors.New("upstream server error") ErrUpstreamTimeout = errors.New("upstream request timeout") + ErrTooManyConnections = errors.New("too many concurrent upstream connections") ) // Internal fetcher errors. @@ -122,6 +129,9 @@ type Config struct { AllowHTTP bool // MaxConnectionsPerHost limits concurrent connections to each upstream host. MaxConnectionsPerHost int + // MaxConnections limits concurrent connections to all upstream hosts + // together. + MaxConnections int // BlockedNetworks are operator-supplied CIDR ranges refused by the // dialer, in addition to the always-enforced built-in ranges. BlockedNetworks []netip.Prefix @@ -143,15 +153,22 @@ func DefaultConfig() *Config { }, AllowHTTP: false, MaxConnectionsPerHost: DefaultMaxConnectionsPerHost, + MaxConnections: DefaultMaxConnections, } } -// HTTPFetcher implements Fetcher with SSRF protection and per-host connection limits. +// HTTPFetcher implements Fetcher with SSRF protection and connection limits +// per host and for all hosts together. type HTTPFetcher struct { client *http.Client config *Config hostSems map[string]chan struct{} // per-host semaphores hostSemMu sync.Mutex // protects hostSems map + // allHostsSemaphore has one slot per connection allowed to all hosts + // together (config.MaxConnections). + allHostsSemaphore chan struct{} + // connectionWaitTimeout is ConnectionWaitTimeout; tests shorten it. + connectionWaitTimeout time.Duration } // New creates a new HTTPFetcher with SSRF protection. @@ -192,13 +209,18 @@ func New(config *Config) *HTTPFetcher { } return &HTTPFetcher{ - client: client, - config: config, - hostSems: make(map[string]chan struct{}), + client: client, + config: config, + hostSems: make(map[string]chan struct{}), + allHostsSemaphore: make(chan struct{}, config.MaxConnections), + connectionWaitTimeout: ConnectionWaitTimeout, } } -// Fetch retrieves content from the given URL with SSRF protection. +// Fetch retrieves content from the given URL with SSRF protection. When +// MaxConnections fetches are already in progress, it waits up to +// ConnectionWaitTimeout for one to finish, then fails with +// ErrTooManyConnections. func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*FetchResult, error) { // Validate URL before making request err := validateURL(ctx, url, f.config.AllowHTTP) @@ -206,24 +228,17 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*FetchResult, erro return nil, err } - // Extract host for rate limiting - host := extractHost(url) - - // Acquire semaphore slot for this host - sem := f.getHostSemaphore(host) - select { - case sem <- struct{}{}: - // Acquired slot - case <-ctx.Done(): - return nil, ctx.Err() + release, err := f.acquireConnection(ctx, extractHost(url)) + if err != nil { + return nil, err } - // If we fail before returning a result, release the slot + // If we fail before returning a result, release the connection success := false defer func() { if !success { - <-sem + release() } }() @@ -267,17 +282,52 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*FetchResult, erro return nil, fmt.Errorf("upstream request failed: %w", err) } - result, err := f.buildResult(resp, remoteAddr, fetchDuration, sem) + result, err := f.buildResult(resp, remoteAddr, fetchDuration, release) if err != nil { return nil, err } - // Mark success so defer doesn't release the semaphore + // Mark success so defer doesn't release the connection; closing the + // result's Content does success = true return result, nil } +// acquireConnection takes a slot for host, then one of the slots shared by +// all hosts, and returns the func that gives both back. The host's slot +// comes first, so fetches queued for one busy host hold no shared slot. +// Only the wait for a shared slot is bounded: after connectionWaitTimeout +// it fails with ErrTooManyConnections. +func (f *HTTPFetcher) acquireConnection( + ctx context.Context, host string, +) (func(), error) { + hostSem := f.getHostSemaphore(host) + + select { + case hostSem <- struct{}{}: + case <-ctx.Done(): + return nil, ctx.Err() + } + + select { + case f.allHostsSemaphore <- struct{}{}: + case <-time.After(f.connectionWaitTimeout): + <-hostSem + + return nil, ErrTooManyConnections + case <-ctx.Done(): + <-hostSem + + return nil, ctx.Err() + } + + return func() { + <-hostSem + <-f.allHostsSemaphore + }, nil +} + // getHostSemaphore returns the semaphore for a host, creating it if necessary. func (f *HTTPFetcher) getHostSemaphore(host string) chan struct{} { f.hostSemMu.Lock() @@ -293,12 +343,12 @@ func (f *HTTPFetcher) getHostSemaphore(host string) chan struct{} { } // buildResult validates the upstream response and assembles a FetchResult -// whose Content releases the host semaphore slot when closed. +// whose Content calls release when closed. func (f *HTTPFetcher) buildResult( resp *http.Response, remoteAddr string, fetchDuration time.Duration, - sem chan struct{}, + release func(), ) (*FetchResult, error) { // Extract HTTP version (strip "HTTP/" prefix) httpVersion := strings.TrimPrefix(resp.Proto, "HTTP/") @@ -333,7 +383,7 @@ func (f *HTTPFetcher) buildResult( } return &FetchResult{ - Content: &semaphoreReleasingReadCloser{limitedBody, resp.Body, sem}, + Content: &semaphoreReleasingReadCloser{limitedBody, resp.Body, release}, ContentLength: resp.ContentLength, ContentType: contentType, Headers: resp.Header, @@ -574,17 +624,18 @@ func (r *limitedReader) Read(p []byte) (int, error) { return n, err } -// semaphoreReleasingReadCloser releases a semaphore slot when closed. +// semaphoreReleasingReadCloser releases the fetch's connection slots when +// closed. type semaphoreReleasingReadCloser struct { *limitedReader - closer io.Closer - sem chan struct{} + closer io.Closer + release func() } func (r *semaphoreReleasingReadCloser) Close() error { err := r.closer.Close() - <-r.sem // Release semaphore slot + r.release() return err } diff --git a/internal/httpfetcher/max_connections_internal_test.go b/internal/httpfetcher/max_connections_internal_test.go new file mode 100644 index 0000000..d23816c --- /dev/null +++ b/internal/httpfetcher/max_connections_internal_test.go @@ -0,0 +1,165 @@ +package httpfetcher + +import ( + "context" + "errors" + "net" + "strconv" + "testing" + "time" +) + +// imageURLOnPort is the fake upstream's image route on testPublicHost at +// port. Each port is a different host to the per-host limit, while the test +// dialer sends every port to the one test server. +func imageURLOnPort(port int) string { + return "http://" + net.JoinHostPort(testPublicHost, strconv.Itoa(port)) + + "/image" +} + +func TestDefaultConfigMaxConnections(t *testing.T) { + t.Parallel() + + if got := DefaultConfig().MaxConnections; got != DefaultMaxConnections { + t.Errorf("MaxConnections = %d, want %d", got, DefaultMaxConnections) + } +} + +// TestFetchLimitsConnectionsToAllHostsTogether checks that MaxConnections +// counts the fetches to every host together, apart from the per-host +// limit: with MaxConnections at 2 and two responses open from two hosts, a +// fetch from a third host, which has nothing open, waits the whole wait +// timeout and fails with ErrTooManyConnections. Closing one response lets +// it through. +func TestFetchLimitsConnectionsToAllHostsTogether(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + + cfg := DefaultConfig() + cfg.MaxConnections = 2 + + f, _ := newServerFetcher(t, srv, cfg) + f.connectionWaitTimeout = 100 * time.Millisecond + + first, err := f.Fetch(testContext(t), imageURLOnPort(81)) + if err != nil { + t.Fatalf("first Fetch() error = %v", err) + } + + second, err := f.Fetch(testContext(t), imageURLOnPort(82)) + if err != nil { + t.Fatalf("second Fetch() error = %v", err) + } + + defer func() { _ = second.Content.Close() }() + + start := time.Now() + + _, err = f.Fetch(testContext(t), imageURLOnPort(83)) + if !errors.Is(err, ErrTooManyConnections) { + t.Fatalf("third Fetch() error = %v, want ErrTooManyConnections", err) + } + + if waited := time.Since(start); waited < f.connectionWaitTimeout { + t.Errorf("third Fetch() failed after %v, before waiting %v", + waited, f.connectionWaitTimeout) + } + + if held := semLen(f, testPublicHost+":83"); held != 0 { + t.Errorf("the refused fetch kept its host's slot: %d held", held) + } + + err = first.Content.Close() + if err != nil { + t.Fatalf("close first body: %v", err) + } + + third, err := f.Fetch(testContext(t), imageURLOnPort(83)) + if err != nil { + t.Fatalf("Fetch() after a response was closed: error = %v", err) + } + + _ = third.Content.Close() +} + +// TestFetchFreesHostSlotWhenContextEndsWaitingForConnection checks that a +// fetch whose request context ends while it waits for a connection shared +// by all hosts gives its host's slot back. With MaxConnections at 1 and one +// response open, a fetch from another host takes that host's slot and waits; +// its context ends long before the 10 second wait timeout. +func TestFetchFreesHostSlotWhenContextEndsWaitingForConnection(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + + cfg := DefaultConfig() + cfg.MaxConnections = 1 + + f, _ := newServerFetcher(t, srv, cfg) + + first, err := f.Fetch(testContext(t), imageURLOnPort(81)) + if err != nil { + t.Fatalf("first Fetch() error = %v", err) + } + + defer func() { _ = first.Content.Close() }() + + ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) + defer cancel() + + _, err = f.Fetch(ctx, imageURLOnPort(82)) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("second Fetch() error = %v, want context.DeadlineExceeded", err) + } + + if held := semLen(f, testPublicHost+":82"); held != 0 { + t.Errorf("the fetch kept its host's slot after its context ended: "+ + "%d held", held) + } +} + +// TestFetchReleasesConnectionOnError checks that a fetch that fails after +// taking its connection gives it back: with MaxConnections at 1, the slot +// must be free after the failure and the next fetch must succeed. +func TestFetchReleasesConnectionOnError(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + url string + want error + }{ + {"upstream answers 500", upstreamURL("/status/500"), ErrUpstreamError}, + {"upstream sends HTML", upstreamURL("/html"), ErrInvalidContentType}, + // 198.51.100.7 (TEST-NET-2) passes the SSRF checks, and the test + // dialer refuses every host but testPublicHost. + {"connecting fails", "http://198.51.100.7/image", errUnexpectedDial}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + srv := startUpstream(t) + + cfg := DefaultConfig() + cfg.MaxConnections = 1 + + f, _ := newServerFetcher(t, srv, cfg) + f.connectionWaitTimeout = 100 * time.Millisecond + + _, err := f.Fetch(testContext(t), tc.url) + if !errors.Is(err, tc.want) { + t.Fatalf("Fetch() error = %v, want %v", err, tc.want) + } + + if held := len(f.allHostsSemaphore); held != 0 { + t.Fatalf("connection still held after the error: %d held", held) + } + + res := fetchImage(t, f, "/image") + _ = res.Content.Close() + }) + } +} diff --git a/internal/imageprocessor/imageprocessor.go b/internal/imageprocessor/imageprocessor.go index dd77ac1..6a9a92d 100644 --- a/internal/imageprocessor/imageprocessor.go +++ b/internal/imageprocessor/imageprocessor.go @@ -7,7 +7,9 @@ import ( "errors" "fmt" "io" + "runtime" "sync" + "time" "github.com/davidbyttow/govips/v2/vips" ) @@ -17,11 +19,21 @@ import ( //nolint:gochecknoglobals // package-level sync.Once for one-time vips init var vipsOnce sync.Once -// initVips initializes libvips with quiet logging. +// initVips initializes libvips with quiet logging, one worker thread per +// image and no operation cache. Process already works on one image per CPU +// by default, so more threads per image would only compete for the CPUs. +// Each request decodes different source bytes, so the operation cache +// would rarely be hit and would hold memory outside MaxConcurrentProcessing; +// repeated requests are served from pixa's disk cache instead. func initVips() { vipsOnce.Do(func() { vips.LoggingSettings(nil, vips.LogLevelError) - vips.Startup(nil) + vips.Startup(&vips.Config{ + ConcurrencyLevel: 1, + MaxCacheSize: 0, + MaxCacheMem: 0, + MaxCacheFiles: 0, + }) }) } @@ -106,9 +118,23 @@ var ErrInputDataTooLarge = errors.New("input data exceeds maximum allowed size") // not supported. var ErrUnsupportedOutputFormat = errors.New("unsupported output format") +// ErrTooManyImages is returned when MaxConcurrentProcessing images are being +// processed and none finishes within ProcessingWaitTimeout. +var ErrTooManyImages = errors.New("too many images being processed at once") + +// ProcessingWaitTimeout is how long Process waits for a free slot when +// MaxConcurrentProcessing images are already being processed. +const ProcessingWaitTimeout = 10 * time.Second + // ImageProcessor implements image transformation using libvips via govips. type ImageProcessor struct { maxInputBytes int64 + // processingSemaphore has one slot per image that may be processed at + // once. Process holds a slot from before it reads its input until it + // returns, so the input, the decoded image and the output all count. + processingSemaphore chan struct{} + // processingWaitTimeout is ProcessingWaitTimeout; tests shorten it. + processingWaitTimeout time.Duration } // Params holds configuration for creating an ImageProcessor. @@ -117,6 +143,9 @@ type Params struct { // MaxInputBytes is the maximum allowed input size in bytes. // If <= 0, DefaultMaxInputBytes is used. MaxInputBytes int64 + // MaxConcurrentProcessing is the most images processed at once. + // If <= 0, the number of CPUs Go uses (runtime.GOMAXPROCS(0)) is used. + MaxConcurrentProcessing int } // New creates a new image processor with the given parameters. @@ -129,17 +158,34 @@ func New(params Params) *ImageProcessor { maxInputBytes = DefaultMaxInputBytes } + maxConcurrentProcessing := params.MaxConcurrentProcessing + if maxConcurrentProcessing <= 0 { + maxConcurrentProcessing = runtime.GOMAXPROCS(0) + } + return &ImageProcessor{ - maxInputBytes: maxInputBytes, + maxInputBytes: maxInputBytes, + processingSemaphore: make(chan struct{}, maxConcurrentProcessing), + processingWaitTimeout: ProcessingWaitTimeout, } } -// Process transforms an image according to the request. +// Process transforms an image according to the request. When +// MaxConcurrentProcessing images are already being processed, it waits up +// to ProcessingWaitTimeout for one to finish, then fails with +// ErrTooManyImages. func (p *ImageProcessor) Process( - _ context.Context, + ctx context.Context, input io.Reader, req *Request, ) (*Result, error) { + release, err := p.acquireSlot(ctx) + if err != nil { + return nil, err + } + + defer release() + // Read input with a size limit to prevent unbounded memory consumption. // We read at most maxInputBytes+1 so we can detect if the input exceeds // the limit without consuming additional memory. @@ -285,6 +331,29 @@ func FormatToMIME(format Format) string { } } +// acquireSlot takes a slot in processingSemaphore, waiting at most +// processingWaitTimeout for one to free up, and returns the func that gives +// it back. A free slot is taken even when ctx has ended; only the wait for +// one stops when ctx ends, as the rest of Process does not check ctx. +func (p *ImageProcessor) acquireSlot(ctx context.Context) (func(), error) { + release := func() { <-p.processingSemaphore } + + select { + case p.processingSemaphore <- struct{}{}: + return release, nil + default: + } + + select { + case p.processingSemaphore <- struct{}{}: + return release, nil + case <-time.After(p.processingWaitTimeout): + return nil, ErrTooManyImages + case <-ctx.Done(): + return nil, ctx.Err() + } +} + // detectFormat returns the format string from a vips image. func (p *ImageProcessor) detectFormat(img *vips.ImageRef) string { format := img.Format() diff --git a/internal/imageprocessor/max_concurrent_processing_internal_test.go b/internal/imageprocessor/max_concurrent_processing_internal_test.go new file mode 100644 index 0000000..dc826c4 --- /dev/null +++ b/internal/imageprocessor/max_concurrent_processing_internal_test.go @@ -0,0 +1,302 @@ +package imageprocessor + +import ( + "bytes" + "context" + "errors" + "io" + "runtime" + "strings" + "sync" + "testing" + "testing/iotest" + "time" +) + +// errTestReadFailed is the error the unreadable test input returns. +var errTestReadFailed = errors.New("test input cannot be read") + +// readingCounter counts the Process calls reading their input at the same +// time and remembers the most there ever were. +type readingCounter struct { + mu sync.Mutex + reading int + most int +} + +func (c *readingCounter) start() { + c.mu.Lock() + defer c.mu.Unlock() + + c.reading++ + c.most = max(c.most, c.reading) +} + +func (c *readingCounter) stop() { + c.mu.Lock() + defer c.mu.Unlock() + + c.reading-- +} + +func (c *readingCounter) mostReading() int { + c.mu.Lock() + defer c.mu.Unlock() + + return c.most +} + +// gatedReader is a Process input. Its first Read counts the call in, +// reports it on entered and blocks until gate is closed; it counts the call +// out when it returns io.EOF. Process reads its input only while it holds a +// processing slot, so the count never goes above MaxConcurrentProcessing. +type gatedReader struct { + data *bytes.Reader + gate <-chan struct{} + entered chan<- struct{} + counter *readingCounter + started bool +} + +func (r *gatedReader) Read(p []byte) (int, error) { + if !r.started { + r.started = true + r.counter.start() + + r.entered <- struct{}{} + + <-r.gate + } + + n, err := r.data.Read(p) + if errors.Is(err, io.EOF) { + r.counter.stop() + } + + return n, err +} + +// smallJPEGRequest asks for a 5x5 JPEG. +func smallJPEGRequest() *Request { + return &Request{ + Size: Size{Width: 5, Height: 5}, + Format: FormatJPEG, + Quality: 85, + FitMode: FitCover, + } +} + +// processInBackground runs Process on reader in a new goroutine and sends +// its error on results. +func processInBackground( + proc *ImageProcessor, reader *gatedReader, results chan<- error, +) { + go func() { + result, err := proc.Process(context.Background(), reader, smallJPEGRequest()) + if err == nil { + _ = result.Content.Close() + } + + results <- err + }() +} + +// waitForEntries fails the test unless count Process calls report on +// entered within a few seconds. +func waitForEntries(t *testing.T, entered <-chan struct{}, count int) { + t.Helper() + + for range count { + select { + case <-entered: + case <-time.After(5 * time.Second): + t.Fatal("Process calls did not start reading their input") + } + } +} + +func TestNewDefaultsMaxConcurrentProcessingToCPUs(t *testing.T) { + t.Parallel() + + for _, limit := range []int{0, -1} { + proc := New(Params{MaxConcurrentProcessing: limit}) + if got := cap(proc.processingSemaphore); got != runtime.GOMAXPROCS(0) { + t.Errorf("MaxConcurrentProcessing %d: %d slots, want %d, one per CPU", + limit, got, runtime.GOMAXPROCS(0)) + } + } + + proc := New(Params{MaxConcurrentProcessing: 3}) + if got := cap(proc.processingSemaphore); got != 3 { + t.Errorf("MaxConcurrentProcessing 3: %d slots, want 3", got) + } +} + +// TestProcessNeverExceedsMaxConcurrentProcessing starts more Process calls +// than MaxConcurrentProcessing allows and holds the first ones inside +// Process until the test lets them go. No more than the limit may be +// working at once, and the calls held back must wait for a slot and then +// succeed. +func TestProcessNeverExceedsMaxConcurrentProcessing(t *testing.T) { + t.Parallel() + + const ( + limit = 2 + calls = 6 + ) + + proc := New(Params{MaxConcurrentProcessing: limit}) + input := createTestJPEG(t, 50, 50) + + counter := &readingCounter{} + gate := make(chan struct{}) + entered := make(chan struct{}, calls) + results := make(chan error, calls) + + openGate := sync.OnceFunc(func() { close(gate) }) + t.Cleanup(openGate) + + for range calls { + processInBackground(proc, &gatedReader{ + data: bytes.NewReader(input), gate: gate, entered: entered, + counter: counter, + }, results) + } + + waitForEntries(t, entered, limit) + + // A call beyond the limit would start reading its input now. + select { + case <-entered: + t.Fatalf("a Process call started while %d were already working", limit) + case <-time.After(100 * time.Millisecond): + } + + openGate() + + for range calls { + err := <-results + if err != nil { + t.Errorf("Process() error = %v, want nil once a slot is free", err) + } + } + + if most := counter.mostReading(); most > limit { + t.Errorf("%d Process calls worked at once, want at most %d", most, limit) + } +} + +// TestProcessWaitsThenFailsWhenNoSlotFrees holds the only slot and checks +// that another call waits the whole wait timeout, then fails with +// ErrTooManyImages instead of processing anyway. +func TestProcessWaitsThenFailsWhenNoSlotFrees(t *testing.T) { + t.Parallel() + + proc := New(Params{MaxConcurrentProcessing: 1}) + proc.processingWaitTimeout = 100 * time.Millisecond + + input := createTestJPEG(t, 10, 10) + + gate := make(chan struct{}) + entered := make(chan struct{}, 1) + held := make(chan error, 1) + + openGate := sync.OnceFunc(func() { close(gate) }) + t.Cleanup(openGate) + + processInBackground(proc, &gatedReader{ + data: bytes.NewReader(input), gate: gate, entered: entered, + counter: &readingCounter{}, + }, held) + waitForEntries(t, entered, 1) + + start := time.Now() + + _, err := proc.Process(context.Background(), bytes.NewReader(input), + smallJPEGRequest()) + if !errors.Is(err, ErrTooManyImages) { + t.Fatalf("Process() error = %v, want ErrTooManyImages", err) + } + + if waited := time.Since(start); waited < proc.processingWaitTimeout { + t.Errorf("Process() failed after %v, before waiting %v", + waited, proc.processingWaitTimeout) + } + + openGate() + + err = <-held + if err != nil { + t.Errorf("Process() holding the slot: error = %v, want nil", err) + } +} + +// TestProcessReleasesSlotOnError checks that Process gives its slot back +// when it fails, whether it fails early or late: with one slot, the slot +// must be free after the failure and the next call must succeed. +func TestProcessReleasesSlotOnError(t *testing.T) { + t.Parallel() + + valid := createTestJPEG(t, 10, 10) + + unsupported := smallJPEGRequest() + unsupported.Format = "bmp" + + cases := []struct { + name string + input io.Reader + req *Request + // want is the error Process must return; nil means any error. + want error + }{ + { + name: "input cannot be read", + input: iotest.ErrReader(errTestReadFailed), + req: smallJPEGRequest(), + want: errTestReadFailed, + }, + { + name: "input over the byte limit", + input: bytes.NewReader(createTestJPEG(t, 800, 600)), + req: smallJPEGRequest(), + want: ErrInputDataTooLarge, + }, + { + name: "input not an image", + input: strings.NewReader("not an image"), + req: smallJPEGRequest(), + }, + { + name: "output format not supported", + input: bytes.NewReader(valid), + req: unsupported, + want: ErrUnsupportedOutputFormat, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + proc := New(Params{MaxInputBytes: 4096, MaxConcurrentProcessing: 1}) + proc.processingWaitTimeout = 100 * time.Millisecond + + _, err := proc.Process(context.Background(), tc.input, tc.req) + if err == nil || (tc.want != nil && !errors.Is(err, tc.want)) { + t.Fatalf("Process() error = %v, want %v", err, tc.want) + } + + if held := len(proc.processingSemaphore); held != 0 { + t.Fatalf("slot still held after the error: %d held", held) + } + + result, err := proc.Process(context.Background(), bytes.NewReader(valid), + smallJPEGRequest()) + if err != nil { + t.Fatalf("Process() after the error = %v, want nil", err) + } + + _ = result.Content.Close() + }) + } +} diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index e15b90a..ddac1b6 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -383,13 +383,16 @@ func (c *Cache) GetSourceMetadataID( return id, nil } -// GetSourceContent returns a reader for cached source content by its hash. -func (c *Cache) GetSourceContent(contentHash ContentHash) (io.ReadCloser, error) { +// GetSourceContent returns a reader for cached source content by its hash, +// and the content's size in bytes. +func (c *Cache) GetSourceContent( + contentHash ContentHash, +) (io.ReadCloser, int64, error) { if c.disabled { - return nil, ErrNotFound + return nil, 0, ErrNotFound } - return c.srcContent.Load(contentHash) + return c.srcContent.LoadWithSize(contentHash) } // CleanExpired removes expired entries from the cache. diff --git a/internal/imgcache/max_concurrent_processing_internal_test.go b/internal/imgcache/max_concurrent_processing_internal_test.go new file mode 100644 index 0000000..e4c310e --- /dev/null +++ b/internal/imgcache/max_concurrent_processing_internal_test.go @@ -0,0 +1,125 @@ +package imgcache + +import ( + "image/color" + "image/jpeg" + "io" + "os" + "testing" + "time" + + "sneak.berlin/go/pixa/internal/imageprocessor" +) + +// widthOnlyRequest asks for the test photo at width, its height scaled to +// keep the photo's aspect ratio. +func widthOnlyRequest(fixtures *TestFixtures, width int) *ImageRequest { + return &ImageRequest{ + SourceHost: fixtures.GoodHost, + SourcePath: testPathPhoto, + Size: Size{Width: width}, + Format: FormatJPEG, + Quality: 85, + FitMode: FitCover, + } +} + +// holdProcessingSlot takes one of proc's processing slots and returns the +// func that gives it back. Process takes its slot before it reads its input, +// so once it has read a byte from the pipe it holds the slot, until the pipe +// is closed. +func holdProcessingSlot( + t *testing.T, proc *imageprocessor.ImageProcessor, +) func() { + t.Helper() + + input, feed := io.Pipe() + + go func() { + _, _ = proc.Process(t.Context(), input, &imageprocessor.Request{}) + }() + + _, err := feed.Write([]byte{0}) + if err != nil { + t.Fatalf("Process call to hold the slot did not start: %v", err) + } + + release := func() { _ = feed.Close() } + t.Cleanup(release) + + return release +} + +// TestService_Get_WaitsForSlotBeforeReadingCachedSource checks that a +// request whose source is cached holds none of it while it waits for a +// processing slot: it reads the cached file only once it has a slot. With +// the only slot held, a request for a new width of the cached 100x100 photo +// waits; the cached file is then rewritten as a 100x50 image before the slot +// is freed, so the request must answer with that image scaled to 40x20. +func TestService_Get_WaitsForSlotBeforeReadingCachedSource(t *testing.T) { + t.Parallel() + + svc, fixtures := SetupTestService(t) + svc.processor = imageprocessor.New( + imageprocessor.Params{MaxConcurrentProcessing: 1}, + ) + + // A first request caches the photo as a source. + resp, err := svc.Get(t.Context(), widthOnlyRequest(fixtures, 50)) + if err != nil { + t.Fatalf("first Get() error = %v", err) + } + + _ = resp.Content.Close() + + contentHash, _, err := svc.cache.LookupSource(t.Context(), + widthOnlyRequest(fixtures, 50)) + if err != nil || contentHash == "" { + t.Fatalf("LookupSource() = %q, %v; want the cached source", + contentHash, err) + } + + release := holdProcessingSlot(t, svc.processor) + + var ( + waited *ImageResponse + waitedErr error + ) + + done := make(chan struct{}) + + go func() { + defer close(done) + + waited, waitedErr = svc.Get(t.Context(), widthOnlyRequest(fixtures, 40)) + }() + + // Give the request time to reach the slot: had it read the cached source + // before waiting, it would have read it by now. + time.Sleep(100 * time.Millisecond) + + err = os.WriteFile(svc.cache.srcContent.hashToPath(contentHash), + generateTestJPEG(t, 100, 50, color.RGBA{0, 0, 255, 255}), 0o600) + if err != nil { + t.Fatalf("failed to rewrite the cached source: %v", err) + } + + release() + <-done + + if waitedErr != nil { + t.Fatalf("Get() error = %v", waitedErr) + } + + defer func() { _ = waited.Content.Close() }() + + output, err := jpeg.DecodeConfig(waited.Content) + if err != nil { + t.Fatalf("failed to decode the response: %v", err) + } + + if output.Width != 40 || output.Height != 20 { + t.Errorf("response is %dx%d, want 40x20: the request read the cached "+ + "source before it had a processing slot", output.Width, output.Height) + } +} diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index d85dee8..e1152dc 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -43,6 +43,9 @@ type ServiceConfig struct { SigningKey string // Allowlist is the list of hosts that don't require signatures Allowlist []string + // MaxConcurrentProcessing is the most images processed at once; zero + // uses the image processor's default, one per CPU + MaxConcurrentProcessing int // Logger for logging Logger *slog.Logger } @@ -91,9 +94,10 @@ func NewService(cfg *ServiceConfig) (*Service, error) { } maxResponseSize := fetcherCfg.MaxResponseSize - processor := imageprocessor.New( - imageprocessor.Params{MaxInputBytes: maxResponseSize}, - ) + processor := imageprocessor.New(imageprocessor.Params{ + MaxInputBytes: maxResponseSize, + MaxConcurrentProcessing: cfg.MaxConcurrentProcessing, + }) return &Service{ cache: cfg.Cache, @@ -237,38 +241,37 @@ func (s *Service) GenerateSignedURL( baseURL, path, sig, exp, req.Quality, req.FitMode), nil } -// loadCachedSource attempts to load source content from cache, returning nil -// if the cached data is unavailable or exceeds maxResponseSize. -func (s *Service) loadCachedSource(contentHash ContentHash) []byte { - reader, err := s.cache.GetSourceContent(contentHash) +// loadCachedSource opens source content from cache, without reading it, and +// returns it with its size; nil if the cached data is unavailable, empty or +// exceeds maxResponseSize. +func (s *Service) loadCachedSource( + contentHash ContentHash, +) (io.ReadCloser, int64) { + reader, size, err := s.cache.GetSourceContent(contentHash) if err != nil { s.log.Warn("failed to load cached source, fetching", "error", err) - return nil + return nil, 0 } - // Bound the read to maxResponseSize to prevent unbounded memory use - // from unexpectedly large cached files. - limited := io.LimitReader(reader, s.maxResponseSize+1) - data, err := io.ReadAll(limited) - _ = reader.Close() + if size > s.maxResponseSize { + _ = reader.Close() - if err != nil { - s.log.Warn("failed to read cached source, fetching", "error", err) - - return nil - } - - if int64(len(data)) > s.maxResponseSize { s.log.Warn("cached source exceeds max response size, discarding", "hash", contentHash, "max_bytes", s.maxResponseSize, ) - return nil + return nil, 0 } - return data + if size == 0 { + _ = reader.Close() + + return nil, 0 + } + + return reader, size } // processFromSourceOrFetch processes an image, using cached source content @@ -285,22 +288,27 @@ func (s *Service) processFromSourceOrFetch( s.log.Warn("source lookup failed", "error", err) } - var sourceData []byte + var ( + source io.ReadCloser + sourceSize int64 + ) if contentHash != "" { s.log.Debug("using cached source", "hash", contentHash) - sourceData = s.loadCachedSource(contentHash) + source, sourceSize = s.loadCachedSource(contentHash) } // Fetch from upstream if we don't have source data or it's empty - if len(sourceData) == 0 { + if source == nil { return s.fetchAndProcess(ctx, req, cacheKey) } - // Process using cached source; nothing was fetched from upstream - resp, err := s.processAndStore( - ctx, req, cacheKey, sourceData, int64(len(sourceData)), - ) + defer func() { _ = source.Close() }() + + // Process using cached source; nothing was fetched from upstream. The + // image processor reads the source only once it has a processing slot, + // so a request waiting for one holds none of it in memory. + resp, err := s.processAndStore(ctx, req, cacheKey, source, sourceSize) return resp, 0, err } @@ -334,6 +342,10 @@ func (s *Service) fetchAndProcess( return nil, 0, fmt.Errorf("upstream fetch failed: %w", err) } + // Closing the body frees the upstream connection. It is closed only + // after processing, so the fetcher's connection limit also bounds the + // fetched sources held in memory while their requests wait for a + // processing slot. defer func() { _ = fetchResult.Content.Close() }() // Read and validate the source content @@ -379,17 +391,20 @@ func (s *Service) fetchAndProcess( // Continue even if caching fails } - resp, err := s.processAndStore(ctx, req, cacheKey, sourceData, fetchBytes) + resp, err := s.processAndStore( + ctx, req, cacheKey, bytes.NewReader(sourceData), fetchBytes, + ) return resp, fetchBytes, err } -// processAndStore processes an image and stores the result. +// processAndStore processes the image read from source and stores the +// result. func (s *Service) processAndStore( ctx context.Context, req *ImageRequest, cacheKey VariantKey, - sourceData []byte, + source io.Reader, fetchBytes int64, ) (*ImageResponse, error) { // Process the image @@ -402,7 +417,7 @@ func (s *Service) processAndStore( FitMode: imageprocessor.FitMode(req.FitMode), } - processResult, err := s.processor.Process(ctx, bytes.NewReader(sourceData), processReq) + processResult, err := s.processor.Process(ctx, source, processReq) if err != nil { return nil, fmt.Errorf("image processing failed: %w", err) } -- 2.54.0 From 99735f479b70db82ecdb56ab33e383fff662d751 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 11:01:30 +0200 Subject: [PATCH 09/11] Answer image requests with 503 in maintenance mode (closes #71) maintenance_mode was only reported by the health check; every request was still served. One middleware in routes.go, applied to /v1/image/ and /v1/e/ only, now answers them with 503, a Retry-After of MaintenanceRetryAfterSeconds and the JSON error body while it is on. It calls Server.MaintenanceMode(), which had no caller. The health check stays 200 and reports maintenance_mode: the image's Docker HEALTHCHECK requests it, a 503 there would make the container unhealthy, and upaas marks a deploy failed when its container is unhealthy. The login and URL generator pages and /metrics keep working. Documented in README.md and config.example.yml. Model: opus-5-5 --- README.md | 9 +- TODO.md | 8 + config.example.yml | 6 + .../server/login_rate_limit_internal_test.go | 10 +- internal/server/maintenance_internal_test.go | 169 ++++++++++++++++++ internal/server/routes.go | 57 +++++- 6 files changed, 249 insertions(+), 10 deletions(-) create mode 100644 internal/server/maintenance_internal_test.go diff --git a/README.md b/README.md index 001d863..808d192 100644 --- a/README.md +++ b/README.md @@ -245,7 +245,7 @@ variables set by the file's `env:` section are checked the same way. | `PIXA_METRICS_PASSWORD` | `metrics.password` | Password for `/metrics`; set together with the username | | `PIXA_SENTRY_DSN` | `sentry_dsn` | Sentry DSN for error reporting; empty disables it | | `PIXA_DEBUG` | `debug` | Debug logging and plain-HTTP local development; default `false` | -| `PIXA_MAINTENANCE_MODE` | `maintenance_mode` | Maintenance flag reported by the health check; default `false` | +| `PIXA_MAINTENANCE_MODE` | `maintenance_mode` | Answer image requests with 503; the health check stays 200; default `false` | Key settings in more detail: @@ -306,6 +306,13 @@ Key settings in more detail: container's CPU limit. A request that finds all of them in use waits up to 10 seconds for one to free up; if none does, and `downstream_timeout` has not ended first, it is answered 503 the same way +- `maintenance_mode` — while `true`, the image routes (`/v1/image/` and + `/v1/e/`) answer every request with 503, a `Retry-After` header and a JSON + error body. The health check (`/.well-known/healthcheck.json`) still answers + 200 and reports `"maintenance_mode": true`. It stays 200 because the image's + Docker `HEALTHCHECK` requests it: a 503 there would make the container + unhealthy, and upaas marks a deploy failed when its container is unhealthy. + The login and URL generator pages and `/metrics` keep working See `config.example.yml` for all options with defaults. diff --git a/TODO.md b/TODO.md index 8276df8..4674052 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,14 @@ P2: security: referer blacklist # Completed Steps +- 2026-09-29 maintenance mode refuses image requests (closes #71): while + `maintenance_mode` is on, `/v1/image/` and `/v1/e/` answer 503 with a + `Retry-After` header and the JSON error body, from one middleware in + `internal/server/routes.go`; the health check stays 200 and reports + `maintenance_mode`, as the image's Docker `HEALTHCHECK` requests it and upaas + marks a deploy failed when its container is unhealthy; the login and URL + generator pages and `/metrics` keep working; documented in `README.md` and + `config.example.yml`. - 2026-09-29 bound concurrent image processing and upstream fetches (closes #64): `max_concurrent_processing` (default the number of CPUs pixa can use) limits the images decoded and encoded at once, and `upstream_connections` diff --git a/config.example.yml b/config.example.yml index d64d77e..ebbc833 100644 --- a/config.example.yml +++ b/config.example.yml @@ -16,6 +16,12 @@ # Server settings port: 8080 debug: false + +# While true, the image routes (/v1/image/ and /v1/e/) answer every request +# with 503 and a Retry-After header. The health check keeps answering 200 and +# reports maintenance_mode as true. It stays 200 because the image's Docker +# HEALTHCHECK requests it: a 503 there would make the container unhealthy, and +# upaas marks a deploy failed when its container is unhealthy. maintenance_mode: false # Data directory for SQLite database and cache files diff --git a/internal/server/login_rate_limit_internal_test.go b/internal/server/login_rate_limit_internal_test.go index c37d86e..f5d8c81 100644 --- a/internal/server/login_rate_limit_internal_test.go +++ b/internal/server/login_rate_limit_internal_test.go @@ -18,6 +18,7 @@ import ( "sneak.berlin/go/pixa/internal/database" "sneak.berlin/go/pixa/internal/globals" "sneak.berlin/go/pixa/internal/handlers" + "sneak.berlin/go/pixa/internal/healthcheck" "sneak.berlin/go/pixa/internal/logger" "sneak.berlin/go/pixa/internal/middleware" ) @@ -71,8 +72,15 @@ func newTestServer(t *testing.T) *Server { t.Fatalf("database.New() error = %v", err) } + hc, err := healthcheck.New(lc, healthcheck.Params{ + Globals: &globals.Globals{}, Config: cfg, Logger: log, Database: db, + }) + if err != nil { + t.Fatalf("healthcheck.New() error = %v", err) + } + h, err := handlers.New(lc, handlers.Params{ - Logger: log, Database: db, Config: cfg, + Logger: log, Healthcheck: hc, Database: db, Config: cfg, }) if err != nil { t.Fatalf("handlers.New() error = %v", err) diff --git a/internal/server/maintenance_internal_test.go b/internal/server/maintenance_internal_test.go new file mode 100644 index 0000000..96b7272 --- /dev/null +++ b/internal/server/maintenance_internal_test.go @@ -0,0 +1,169 @@ +package server + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "sneak.berlin/go/pixa/internal/healthcheck" +) + +// unsignedImagePath is an image URL that carries no signature. +const unsignedImagePath = "/v1/image/cdn.example.com/cat.jpg/100x100.jpeg" + +// TestMaintenanceModeRefusesImageRequests verifies that while maintenance +// mode is on, both image routes answer 503 Service Unavailable with a +// Retry-After header and the JSON error body the image handlers send. +func TestMaintenanceModeRefusesImageRequests(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + s.config.MaintenanceMode = true + + requests := []struct { + method string + path string + }{ + {http.MethodGet, unsignedImagePath}, + {http.MethodHead, unsignedImagePath}, + {http.MethodGet, "/v1/e/token/cat.jpg"}, + } + + for _, tc := range requests { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), tc.method, tc.path, nil)) + t.Logf("status %d, body %s", rec.Code, rec.Body.String()) + + if rec.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want %d", + rec.Code, http.StatusServiceUnavailable) + } + + retryAfter := rec.Header().Get("Retry-After") + + seconds, err := strconv.Atoi(retryAfter) + if err != nil || seconds <= 0 { + t.Errorf("Retry-After = %q, want a positive number of seconds", + retryAfter) + } + + // A HEAD response carries no body. + if tc.method == http.MethodHead { + return + } + + var body struct { + Error string `json:"error"` + Status int `json:"status"` + Timestamp string `json:"timestamp"` + } + + err = json.NewDecoder(rec.Body).Decode(&body) + if err != nil { + t.Fatalf("body is not JSON: %v", err) + } + + if body.Error == "" || body.Status != http.StatusServiceUnavailable || + body.Timestamp == "" { + t.Errorf("body = %+v, want an error, status %d and a timestamp", + body, http.StatusServiceUnavailable) + } + }) + } +} + +// TestImageRequestsServedWithoutMaintenanceMode verifies that while +// maintenance mode is off, image requests reach the image handlers instead +// of the 503. The handlers refuse an unsigned image URL with 401 and a token +// they cannot decrypt with 400, so either status shows a request got through. +func TestImageRequestsServedWithoutMaintenanceMode(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + s.config.MaintenanceMode = false + + requests := []struct { + method string + path string + want int + }{ + {http.MethodGet, unsignedImagePath, http.StatusUnauthorized}, + {http.MethodHead, unsignedImagePath, http.StatusUnauthorized}, + {http.MethodGet, "/v1/e/token/cat.jpg", http.StatusBadRequest}, + } + + for _, tc := range requests { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext( + t.Context(), tc.method, tc.path, nil)) + t.Logf("status %d, body %s", rec.Code, rec.Body.String()) + + if rec.Code != tc.want { + t.Errorf("status = %d, want %d from the image handler", + rec.Code, tc.want) + } + }) + } +} + +// TestMaintenanceModeKeepsOtherRoutes verifies that while maintenance mode +// is on, the health check still answers 200 and reports it, and the login +// page and /metrics still answer 200. The image's Docker HEALTHCHECK +// requests the health check: a 503 there would make the container +// unhealthy, and upaas marks a deploy failed when its container is +// unhealthy. +func TestMaintenanceModeKeepsOtherRoutes(t *testing.T) { + t.Parallel() + + s := newTestServer(t) + s.config.MaintenanceMode = true + + // /metrics is routed only when its username is set. + s.config.MetricsUsername = "metrics" + s.config.MetricsPassword = "metrics-password" + s.SetupRoutes() + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, httptest.NewRequestWithContext(t.Context(), + http.MethodGet, "/.well-known/healthcheck.json", nil)) + t.Logf("health check status %d, body %s", rec.Code, rec.Body.String()) + + if rec.Code != http.StatusOK { + t.Fatalf("health check status = %d, want %d", rec.Code, http.StatusOK) + } + + var health healthcheck.Response + + err := json.NewDecoder(rec.Body).Decode(&health) + if err != nil || !health.Maintenance { + t.Errorf("health check maintenance_mode = %v (error %v), want true", + health.Maintenance, err) + } + + rec = httptest.NewRecorder() + s.ServeHTTP(rec, clientRequest(t, http.MethodGet, nil, firstClient, "")) + + if rec.Code != http.StatusOK { + t.Errorf("login page status = %d, want %d", rec.Code, http.StatusOK) + } + + req := httptest.NewRequestWithContext(t.Context(), + http.MethodGet, "/metrics", nil) + req.SetBasicAuth(s.config.MetricsUsername, s.config.MetricsPassword) + + rec = httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Errorf("/metrics status = %d, want %d", rec.Code, http.StatusOK) + } +} diff --git a/internal/server/routes.go b/internal/server/routes.go index 4623b75..ade30ee 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -1,7 +1,9 @@ package server import ( + "encoding/json" "net/http" + "strconv" "time" sentryhttp "github.com/getsentry/sentry-go/http" @@ -17,6 +19,10 @@ import ( // make per minute; the next is refused with 429 Too Many Requests. const LoginAttemptsPerMinute = 5 +// MaintenanceRetryAfterSeconds is the Retry-After, in seconds, sent with +// the 503 that the image routes answer while maintenance mode is on. +const MaintenanceRetryAfterSeconds = 300 + // SetupRoutes configures all HTTP routes. func (s *Server) SetupRoutes() { s.router = chi.NewRouter() @@ -68,15 +74,23 @@ func (s *Server) SetupRoutes() { s.router.Get("/logout", s.h.HandleLogout()) - // Main image proxy route - // /v1/image///x. - s.router.Get("/v1/image/*", s.h.HandleImage()) - s.router.Head("/v1/image/*", s.h.HandleImage()) + // Image routes, refused while maintenance mode is on. Only these: the + // image's Docker HEALTHCHECK requests the health check, a 503 there + // would make the container unhealthy, and upaas marks a deploy failed + // when its container is unhealthy. + s.router.Group(func(r chi.Router) { + r.Use(s.refuseDuringMaintenance) - // Encrypted image URL route - // The trailing filename (e.g., /img.jpg) is ignored but helps - // browsers with content type - s.router.Get("/v1/e/{token}/*", s.h.HandleImageEnc()) + // Main image proxy route + // /v1/image///x. + r.Get("/v1/image/*", s.h.HandleImage()) + r.Head("/v1/image/*", s.h.HandleImage()) + + // Encrypted image URL route + // The trailing filename (e.g., /img.jpg) is ignored but helps + // browsers with content type + r.Get("/v1/e/{token}/*", s.h.HandleImageEnc()) + }) // Metrics endpoint with auth if s.config.MetricsUsername != "" { @@ -86,3 +100,30 @@ func (s *Server) SetupRoutes() { }) } } + +// refuseDuringMaintenance answers a request with 503 Service Unavailable, +// a Retry-After header and a JSON error body while maintenance mode is on, +// and passes it on otherwise. The body has the fields of the JSON errors +// the image handlers send. +func (s *Server) refuseDuringMaintenance(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !s.MaintenanceMode() { + next.ServeHTTP(w, r) + + return + } + + w.Header().Set("Retry-After", strconv.Itoa(MaintenanceRetryAfterSeconds)) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusServiceUnavailable) + + err := json.NewEncoder(w).Encode(map[string]any{ + "error": "down for maintenance, try again later", + "status": http.StatusServiceUnavailable, + "timestamp": time.Now().UTC().Format(time.RFC3339), + }) + if err != nil { + s.log.Error("json encode error", "error", err) + } + }) +} -- 2.54.0 From bce8860c2e20950e6c253d49bf491a3f795aa06c Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 12:00:10 +0200 Subject: [PATCH 10/11] Keep variant content types in memory for cache hits (closes #70) Cache.metaCache was declared and never used, so every cache hit read and parsed the variant's .meta file. It is now an LRU (github.com/hashicorp/golang-lru/v2) of up to 10,000 variants' content types, filled by StoreVariant and by a read of a .meta file, so a hit for a variant it holds skips the .meta read. Only a type from a .meta file or a store ever enters memory, never the application/octet-stream fallback, and a stored type is never replaced by an older one from disk. The variant file itself is still opened on every hit, so nothing is served from memory alone. Model: opus-5-5 --- README.md | 5 +- TODO.md | 11 + go.mod | 1 + go.sum | 2 + internal/imgcache/cache.go | 73 +++- internal/imgcache/eviction.go | 10 +- internal/imgcache/metacache_internal_test.go | 349 +++++++++++++++++++ internal/imgcache/storage.go | 36 +- internal/imgcache/testutil_internal_test.go | 1 + 9 files changed, 460 insertions(+), 28 deletions(-) create mode 100644 internal/imgcache/metacache_internal_test.go diff --git a/README.md b/README.md index 808d192..9321291 100644 --- a/README.md +++ b/README.md @@ -86,7 +86,10 @@ prevent abuse, and allowlisted source hosts for open access. Multiple source paths may reference the same content blob; the database tracks references rather than using filesystem refcounting. -In-process caching of request-to-output mappings targets 1-5k r/s. +Toward a target of 1-5k r/s, pixa keeps in memory the content types of +the 10,000 transformed images most recently cached or served, so a +cache hit on one of them reads only the image file from disk and not +the metadata file stored beside it. ### Routes diff --git a/TODO.md b/TODO.md index 4674052..bb548f5 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,17 @@ P2: security: referer blacklist # Completed Steps +- 2026-09-29 variant content types kept in memory (closes #70): + `Cache.metaCache` holds the content types of up to 10,000 variants in an LRU + (`github.com/hashicorp/golang-lru/v2`), filled by `StoreVariant` and by + `GetVariant` after it reads a `.meta` file, where a type `StoreVariant` added + meanwhile is kept over the one read, and never with the + `application/octet-stream` served for a variant without one; for a variant it + holds, `GetVariant` skips the `.meta` read, still opening the variant file and + taking the size from it; eviction removes the entry before deleting the files, + and `GetVariant` removes it when the file will not open; the cap is a + constant, not a setting; the unused `variantMeta` type is gone; `README.md` + describes it. - 2026-09-29 maintenance mode refuses image requests (closes #71): while `maintenance_mode` is on, `/v1/image/` and `/v1/e/` answer 503 with a `Retry-After` header and the JSON error body, from one middleware in diff --git a/go.mod b/go.mod index f5c4c98..4a13f35 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( github.com/go-chi/httprate v0.16.0 github.com/gorilla/csrf v1.7.3 github.com/gorilla/securecookie v1.1.2 + github.com/hashicorp/golang-lru/v2 v2.0.7 github.com/prometheus/client_golang v1.23.2 github.com/slok/go-http-metrics v0.13.0 github.com/spf13/cobra v1.10.2 diff --git a/go.sum b/go.sum index f9a4f3d..c4542f6 100644 --- a/go.sum +++ b/go.sum @@ -228,6 +228,8 @@ github.com/hashicorp/go-version v1.2.1/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09 github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc= github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= +github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k= +github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= github.com/hashicorp/hcl v1.0.1-vault-7 h1:ag5OxFVy3QYTFTJODRzTKVZ6xvdfLLCA1cy/Y6xGI0I= github.com/hashicorp/hcl v1.0.1-vault-7/go.mod h1:XYhtn6ijBSAj6n4YqAaf7RBPS4I06AItNorpy+MoQNM= github.com/hashicorp/logutils v1.0.0/go.mod h1:QIAnNjmIWmVIIkWDTG1z5v++HQmx9WQRO+LraFDTW64= diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index ddac1b6..a4faab4 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -14,6 +14,7 @@ import ( "sync" "time" + lru "github.com/hashicorp/golang-lru/v2" "sneak.berlin/go/pixa/internal/httpfetcher" ) @@ -26,6 +27,10 @@ var ( // HTTP status code for successful fetch. const httpStatusOK = 200 +// metaCacheSize is how many variants' content types metaCache holds. A +// variant not among them is served as before, reading its .meta file. +const metaCacheSize = 10000 + // CacheConfig holds cache configuration. type CacheConfig struct { StateDir string @@ -49,12 +54,6 @@ type CacheConfig struct { Logger *slog.Logger } -// variantMeta stores content type for fast cache hits without reading .meta file. -type variantMeta struct { - ContentType string - Size int64 -} - // Cache implements the caching layer for the image proxy. type Cache struct { db *sql.DB @@ -76,9 +75,10 @@ type Cache struct { evictionStarted bool evictionStopOnce sync.Once - // In-memory cache of variant metadata (content type, size) to avoid - // reading .meta files - metaCache map[VariantKey]variantMeta + // metaCache holds the content types of the variants most recently + // stored or served, so a hit does not read the variant's .meta file. + // It never stands in for the variant file, which is always opened. + metaCache *lru.Cache[VariantKey, string] // contentLocks serializes StoreSource and evictSourceBlob per // content hash, closing the race window between an eviction's row @@ -101,6 +101,11 @@ func NewCache(db *sql.DB, config CacheConfig) (*Cache, error) { log = slog.Default() } + metaCache, err := lru.New[VariantKey, string](metaCacheSize) + if err != nil { + return nil, fmt.Errorf("failed to create variant content type cache: %w", err) + } + c := &Cache{ db: db, config: config, @@ -109,7 +114,7 @@ func NewCache(db *sql.DB, config CacheConfig) (*Cache, error) { evictionPressure: make(chan struct{}, 1), evictionStop: make(chan struct{}), evictionDone: make(chan struct{}), - metaCache: make(map[VariantKey]variantMeta), + metaCache: metaCache, contentLocks: newContentLock(), } @@ -177,13 +182,30 @@ func (c *Cache) Lookup(ctx context.Context, req *ImageRequest) (*LookupResult, e }, nil } -// GetVariant returns a reader, size, and content type for a cached variant. +// GetVariant returns a reader, size, and content type for a cached +// variant. The content type comes from metaCache, or else from the +// variant's .meta file and is then kept in metaCache. A variant with +// no .meta file is served as application/octet-stream, which is not +// kept. func (c *Cache) GetVariant(cacheKey VariantKey) (io.ReadCloser, int64, string, error) { if c.disabled { return nil, 0, "", ErrNotFound } - return c.variants.LoadWithMeta(cacheKey) + contentType, known := c.metaCache.Get(cacheKey) + if !known { + return c.loadVariantWithMeta(cacheKey) + } + + reader, size, err := c.variants.LoadWithSize(cacheKey) + if err != nil { + // The file is gone, e.g. deleted outside pixa + c.metaCache.Remove(cacheKey) + + return nil, 0, "", err + } + + return reader, size, contentType, nil } // StoreSource stores fetched source content and metadata. On a @@ -286,6 +308,8 @@ func (c *Cache) StoreVariant( return err } + c.metaCache.Add(cacheKey, contentType) + _, err = c.db.ExecContext(ctx, ` INSERT INTO variant_content (cache_key, size_bytes, content_type) VALUES (?, ?, ?) @@ -499,6 +523,31 @@ func (c *Cache) IncrementTransformCount(ctx context.Context) { } } +// loadVariantWithMeta is GetVariant for a variant metaCache does not +// hold: it reads the content type from the variant's .meta file and +// keeps it in metaCache, unless a StoreVariant has put one there +// meanwhile, as the store's is newer. A read that finds no .meta file, +// as one can between a store's writing of the variant file and of its +// .meta file, serves application/octet-stream and keeps nothing, so +// metaCache only ever holds a type read from a .meta file or passed to +// StoreVariant. +func (c *Cache) loadVariantWithMeta( + cacheKey VariantKey, +) (io.ReadCloser, int64, string, error) { + reader, size, contentType, err := c.variants.LoadWithMeta(cacheKey) + if err != nil { + return nil, 0, "", err + } + + if contentType == "" { + return reader, size, fallbackContentType, nil + } + + c.metaCache.ContainsOrAdd(cacheKey, contentType) + + return reader, size, contentType, nil +} + // writeMetadataSidecar writes the JSON metadata sidecar of a stored source. // A failure is logged and is otherwise non-fatal; the metadata is in the // database. diff --git a/internal/imgcache/eviction.go b/internal/imgcache/eviction.go index 8bf6edb..5d185ea 100644 --- a/internal/imgcache/eviction.go +++ b/internal/imgcache/eviction.go @@ -39,8 +39,8 @@ const tempFilePrefix = ".tmp-" // to each variant file. const variantMetaSuffix = ".meta" -// fallbackContentType is recorded when a reconciled variant file has -// no readable .meta sidecar. +// fallbackContentType is the content type given to a variant file that +// has no readable .meta sidecar, when it is served or reconciled. const fallbackContentType = "application/octet-stream" // UsageBytes returns the total number of bytes of cache content @@ -271,7 +271,9 @@ func (c *Cache) sourceCandidates(ctx context.Context) ([]evictionCandidate, erro // evictVariant removes one variant: accounting row first, then the // content and .meta files, so the database never references a deleted -// file. +// file. The metaCache entry goes before the files; a GetVariant that +// read them just before may put it back, and the next GetVariant then +// fails to open the file and removes it again. func (c *Cache) evictVariant(ctx context.Context, cacheKey VariantKey) error { _, err := c.db.ExecContext(ctx, `DELETE FROM variant_content WHERE cache_key = ?`, string(cacheKey)) @@ -279,6 +281,8 @@ func (c *Cache) evictVariant(ctx context.Context, cacheKey VariantKey) error { return fmt.Errorf("failed to delete variant accounting row: %w", err) } + c.metaCache.Remove(cacheKey) + err = c.variants.DeleteWithMeta(cacheKey) if err != nil { return err diff --git a/internal/imgcache/metacache_internal_test.go b/internal/imgcache/metacache_internal_test.go new file mode 100644 index 0000000..bade726 --- /dev/null +++ b/internal/imgcache/metacache_internal_test.go @@ -0,0 +1,349 @@ +package imgcache + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "sync" + "testing" + "time" +) + +// webpRequest returns a request for a 100x100 WebP variant of path. +func webpRequest(path string) *ImageRequest { + return &ImageRequest{ + SourceHost: testHostCDN, + SourcePath: path, + Size: Size{Width: 100, Height: 100}, + Format: FormatWebP, + Quality: 85, + FitMode: FitCover, + } +} + +// assertVariantServed checks that GetVariant serves key with the given +// content and the image/webp content type storeEvictionTestVariant stores. +func assertVariantServed(t *testing.T, cache *Cache, key VariantKey, content []byte) { + t.Helper() + + reader, size, contentType, err := cache.GetVariant(key) + if err != nil { + t.Fatalf("GetVariant(%s) error = %v", key, err) + } + + defer func() { _ = reader.Close() }() + + got, err := io.ReadAll(reader) + if err != nil { + t.Fatalf("reading variant %s: %v", key, err) + } + + if !bytes.Equal(got, content) { + t.Errorf("GetVariant(%s) content = %q, want %q", key, got, content) + } + + if size != int64(len(content)) { + t.Errorf("GetVariant(%s) size = %d, want %d", key, size, len(content)) + } + + if contentType != testContentTypeWebP { + t.Errorf("GetVariant(%s) content type = %q, want %q", + key, contentType, testContentTypeWebP) + } +} + +// assertVariantNotFound checks that GetVariant refuses key with +// ErrNotFound. +func assertVariantNotFound(t *testing.T, cache *Cache, key VariantKey) { + t.Helper() + + reader, _, _, err := cache.GetVariant(key) + if err == nil { + _ = reader.Close() + } + + if !errors.Is(err, ErrNotFound) { + t.Errorf("GetVariant(%s) error = %v, want ErrNotFound", key, err) + } +} + +// assertLookupMisses checks that Lookup reports request as a miss. +func assertLookupMisses(t *testing.T, cache *Cache, request *ImageRequest) { + t.Helper() + + lookup, err := cache.Lookup(t.Context(), request) + if err != nil { + t.Fatalf("Lookup(%s) error = %v", request.SourcePath, err) + } + + if lookup.Hit { + t.Errorf("Lookup(%s) is a hit, want a miss", request.SourcePath) + } +} + +// renameFile renames the file at from to to. +func renameFile(t *testing.T, from, to string) { + t.Helper() + + err := os.Rename(from, to) + if err != nil { + t.Fatalf("renaming %s: %v", from, err) + } +} + +// TestSecondHitDoesNotReadMetaFile checks that once a variant has been +// stored or read, a hit takes its content type from memory: with the +// .meta file deleted, GetVariant must still return the stored content +// type rather than the application/octet-stream it uses without one. +func TestSecondHitDoesNotReadMetaFile(t *testing.T) { + t.Parallel() + + cache, _ := newEvictionTestCache(t, 1<<20) + content := []byte("webp variant bytes") + + storeEvictionTestVariant(t, cache, testVariantKeyOne, content) + + // A second Cache on the same state directory starts with nothing in + // memory, as pixad does after a restart, so its first read uses the + // .meta file. + restarted, err := NewCache(cache.db, cache.config) + if err != nil { + t.Fatalf("NewCache() error = %v", err) + } + + assertVariantServed(t, restarted, testVariantKeyOne, content) + + err = os.Remove(cache.variants.keyToPath(testVariantKeyOne) + ".meta") + if err != nil { + t.Fatalf("removing .meta file: %v", err) + } + + assertVariantServed(t, cache, testVariantKeyOne, content) + assertVariantServed(t, restarted, testVariantKeyOne, content) +} + +// TestReadDuringStoreKeepsStoredContentType checks that a GetVariant +// which began before StoreVariant finished cannot replace the content +// type the store kept in memory. Such a read can find the variant file +// but not yet its .meta file, and so gets application/octet-stream. The +// test deletes the .meta file after the store, then runs the part of +// GetVariant that comes after its check of memory. +func TestReadDuringStoreKeepsStoredContentType(t *testing.T) { + t.Parallel() + + cache, _ := newEvictionTestCache(t, 1<<20) + content := []byte("webp variant bytes") + + storeEvictionTestVariant(t, cache, testVariantKeyOne, content) + + err := os.Remove(cache.variants.keyToPath(testVariantKeyOne) + ".meta") + if err != nil { + t.Fatalf("removing .meta file: %v", err) + } + + reader, _, contentType, err := cache.loadVariantWithMeta(testVariantKeyOne) + if err != nil { + t.Fatalf("loadVariantWithMeta(%s) error = %v", testVariantKeyOne, err) + } + + _ = reader.Close() + + t.Logf("the read without a .meta file got content type %q", contentType) + + assertVariantServed(t, cache, testVariantKeyOne, content) +} + +// TestReadOfOlderMetaFileKeepsStoredContentType checks that a read +// which got its content type from a .meta file that StoreVariant had +// not yet rewritten cannot replace the type the store kept in memory. +// The test writes such a .meta file, with a different content type, +// after the store, then runs the part of GetVariant that comes after +// its check of memory. +func TestReadOfOlderMetaFileKeepsStoredContentType(t *testing.T) { + t.Parallel() + + cache, _ := newEvictionTestCache(t, 1<<20) + content := []byte("webp variant bytes") + + storeEvictionTestVariant(t, cache, testVariantKeyOne, content) + + olderMeta, err := json.Marshal(VariantMeta{ + ContentType: testContentTypeJPEG, + Size: int64(len(content)), + }) + if err != nil { + t.Fatalf("encoding .meta file: %v", err) + } + + metaPath := cache.variants.keyToPath(testVariantKeyOne) + ".meta" + + err = os.WriteFile(metaPath, olderMeta, StorageFilePerm) + if err != nil { + t.Fatalf("writing .meta file: %v", err) + } + + reader, _, contentType, err := cache.loadVariantWithMeta(testVariantKeyOne) + if err != nil { + t.Fatalf("loadVariantWithMeta(%s) error = %v", testVariantKeyOne, err) + } + + _ = reader.Close() + + if contentType != testContentTypeJPEG { + t.Fatalf("loadVariantWithMeta(%s) content type = %q, want %q from the .meta file", + testVariantKeyOne, contentType, testContentTypeJPEG) + } + + kept, _ := cache.metaCache.Get(testVariantKeyOne) + if kept != testContentTypeWebP { + t.Errorf("content type in memory = %q, want the stored %q", + kept, testContentTypeWebP) + } +} + +// TestFailedReadDuringStoreKeepsStoredContentType checks that a read +// which found no .meta file cannot leave application/octet-stream in +// memory, even when another read has removed the content type +// StoreVariant kept there. In this order: the store; a read that finds +// the variant in memory but cannot open its file, and so removes it +// from memory; a read that opened the variant file before the store +// wrote its .meta file. Later hits must get the stored content type. +func TestFailedReadDuringStoreKeepsStoredContentType(t *testing.T) { + t.Parallel() + + cache, _ := newEvictionTestCache(t, 1<<20) + content := []byte("webp variant bytes") + variantPath := cache.variants.keyToPath(testVariantKeyOne) + metaPath := variantPath + ".meta" + + storeEvictionTestVariant(t, cache, testVariantKeyOne, content) + + renameFile(t, variantPath, variantPath+".hidden") + assertVariantNotFound(t, cache, testVariantKeyOne) + renameFile(t, variantPath+".hidden", variantPath) + + renameFile(t, metaPath, metaPath+".hidden") + + reader, _, contentType, err := cache.GetVariant(testVariantKeyOne) + if err != nil { + t.Fatalf("GetVariant(%s) error = %v", testVariantKeyOne, err) + } + + _ = reader.Close() + + t.Logf("the read without a .meta file got content type %q", contentType) + + renameFile(t, metaPath+".hidden", metaPath) + + assertVariantServed(t, cache, testVariantKeyOne, content) +} + +// TestEvictedVariantIsNotServed checks that a variant the evictor +// removed is a miss and cannot be read, although it had been stored +// and served before. +func TestEvictedVariantIsNotServed(t *testing.T) { + t.Parallel() + + cache, _ := newEvictionTestCache(t, 1500) + + oldRequest := webpRequest("/old.jpg") + newRequest := webpRequest("/new.jpg") + oldKey := CacheKey(oldRequest) + newKey := CacheKey(newRequest) + oldContent := bytes.Repeat([]byte{0x01}, 1000) + newContent := bytes.Repeat([]byte{0x02}, 1000) + + storeEvictionTestVariant(t, cache, oldKey, oldContent) + storeEvictionTestVariant(t, cache, newKey, newContent) + assertVariantServed(t, cache, oldKey, oldContent) + setVariantLastAccessed(t, cache, oldKey, time.Now().Add(-time.Hour)) + + err := cache.EvictToLimit(t.Context()) + if err != nil { + t.Fatalf("EvictToLimit() error = %v", err) + } + + assertLookupMisses(t, cache, oldRequest) + assertVariantNotFound(t, cache, oldKey) + assertVariantServed(t, cache, newKey, newContent) +} + +// TestVariantDeletedFromDiskIsNotServed checks that a variant whose +// file was deleted by something other than the evictor cannot be read, +// and is a miss afterwards. +func TestVariantDeletedFromDiskIsNotServed(t *testing.T) { + t.Parallel() + + cache, _ := newEvictionTestCache(t, 1<<20) + + request := webpRequest("/deleted.jpg") + key := CacheKey(request) + content := []byte("webp variant bytes") + + storeEvictionTestVariant(t, cache, key, content) + assertVariantServed(t, cache, key, content) + + err := os.Remove(cache.variants.keyToPath(key)) + if err != nil { + t.Fatalf("removing variant file: %v", err) + } + + assertVariantNotFound(t, cache, key) + assertLookupMisses(t, cache, request) +} + +// TestConcurrentVariantStoreReadAndEvict stores, reads and evicts +// variants from several goroutines at once, for the race detector. +func TestConcurrentVariantStoreReadAndEvict(t *testing.T) { + t.Parallel() + + cache, _ := newEvictionTestCache(t, 1<<20) + ctx := t.Context() + + var wg sync.WaitGroup + + for goroutine := range 8 { + wg.Go(func() { + key := VariantKey(fmt.Sprintf("aabbccdd01%02d", goroutine)) + content := []byte(key) + + for range 20 { + err := cache.StoreVariant( + ctx, key, bytes.NewReader(content), testContentTypeWebP) + if err != nil { + t.Errorf("StoreVariant(%s) error = %v", key, err) + + return + } + + reader, _, contentType, err := cache.GetVariant(key) + if err != nil { + t.Errorf("GetVariant(%s) error = %v", key, err) + + return + } + + _ = reader.Close() + + if contentType != testContentTypeWebP { + t.Errorf("GetVariant(%s) content type = %q, want %q", + key, contentType, testContentTypeWebP) + } + + err = cache.evictVariant(ctx, key) + if err != nil { + t.Errorf("evictVariant(%s) error = %v", key, err) + + return + } + + assertVariantNotFound(t, cache, key) + } + }) + } + + wg.Wait() +} diff --git a/internal/imgcache/storage.go b/internal/imgcache/storage.go index b9487ce..7e1cb38 100644 --- a/internal/imgcache/storage.go +++ b/internal/imgcache/storage.go @@ -506,32 +506,44 @@ func (s *VariantStorage) Load(key VariantKey) (io.ReadCloser, error) { return f, nil } -// LoadWithMeta returns a reader, size, and content type for the content at -// the given key. -func (s *VariantStorage) LoadWithMeta( - key VariantKey, -) (io.ReadCloser, int64, string, error) { +// LoadWithSize returns a reader and file size for the content at the +// given key. +func (s *VariantStorage) LoadWithSize(key VariantKey) (io.ReadCloser, int64, error) { path := s.keyToPath(key) - metaPath := path + ".meta" f, err := os.Open(path) //nolint:gosec // path derived from cache key if err != nil { if os.IsNotExist(err) { - return nil, 0, "", ErrNotFound + return nil, 0, ErrNotFound } - return nil, 0, "", fmt.Errorf("failed to open content: %w", err) + return nil, 0, fmt.Errorf("failed to open content: %w", err) } stat, err := f.Stat() if err != nil { _ = f.Close() - return nil, 0, "", fmt.Errorf("failed to stat content: %w", err) + return nil, 0, fmt.Errorf("failed to stat content: %w", err) } - // Load metadata for content type - contentType := "application/octet-stream" // fallback + return f, stat.Size(), nil +} + +// LoadWithMeta returns a reader, size, and content type for the content at +// the given key. The content type is read from the .meta file, and is +// empty when that file is missing or unreadable. +func (s *VariantStorage) LoadWithMeta( + key VariantKey, +) (io.ReadCloser, int64, string, error) { + f, size, err := s.LoadWithSize(key) + if err != nil { + return nil, 0, "", err + } + + var contentType string + + metaPath := s.keyToPath(key) + ".meta" metaData, err := os.ReadFile(metaPath) //nolint:gosec // path derived from cache key if err == nil { @@ -541,7 +553,7 @@ func (s *VariantStorage) LoadWithMeta( } } - return f, stat.Size(), contentType, nil + return f, size, contentType, nil } // Exists checks if content exists at the given key. diff --git a/internal/imgcache/testutil_internal_test.go b/internal/imgcache/testutil_internal_test.go index 1ca7c11..b29f750 100644 --- a/internal/imgcache/testutil_internal_test.go +++ b/internal/imgcache/testutil_internal_test.go @@ -24,6 +24,7 @@ const ( testHostExample = "example.com" testPathCat = "/photos/cat.jpg" testContentTypeJPEG = "image/jpeg" + testContentTypeWebP = "image/webp" testHeaderContentType = "Content-Type" ) -- 2.54.0 From fd7d7ed205d78ab2e3cd028f55cde5e7b0d0ead6 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 12:44:37 +0200 Subject: [PATCH 11/11] Container makes /var/lib/pixa usable before starting pixad (closes #159) The entrypoint now creates /var/lib/pixa if it is missing. When the directory or one of its top-level entries belongs to another user or group, it gives the whole tree to pixad (uid and gid 65532); it then sets the directory's mode to 750 and runs the server as pixad as before. Data left by a run under another uid is taken over this way. Only the top level is checked, so a normal start does not walk the cache; the tree is changed deepest first, so an interrupted start is finished by the next one. "Running under upaas" in README.md no longer tells the operator to create or chown the host directory. Model: opus-5-5 --- README.md | 9 ++------- TODO.md | 7 +++++++ deploy/docker-entrypoint.sh | 18 +++++++++++++----- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 9321291..ed5b3b0 100644 --- a/README.md +++ b/README.md @@ -40,9 +40,8 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app for pixa needs: - **Port:** pixa listens on container port `8080`. - **Volume:** container path `/var/lib/pixa`, where pixa keeps its - database and cache. upaas bind-mounts the host path it is given and - does not create it, so the host directory must exist before the first - deploy. + database and cache. Creating the host directory when it is missing is + upaas's job, tracked in https://git.eeqj.de/sneak/upaas/issues/235. - **Environment variables:** - `PIXA_SIGNING_KEY` (required): secret for signed and encrypted URLs and login, 32+ characters, for example from @@ -58,10 +57,6 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app for pixa needs: `healthy`. The probe uses the port from `PORT` (default `8080`), so a port changed only in a mounted config file is not seen by it: change the port with `PORT`. -- **First run:** create the host directory, owned by root or by uid - `65532` and gid `65532`. The server runs as the container's `pixad` - user, which has that uid and gid, and the container gives the - directory to `pixad` when it starts. ## Rationale diff --git a/TODO.md b/TODO.md index bb548f5..637e3a5 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,13 @@ P2: security: referer blacklist # Completed Steps +- 2026-09-29 the container makes `/var/lib/pixa` usable by itself (closes + #159): `deploy/docker-entrypoint.sh` creates the directory if it is missing, + gives the directory and everything in it to `pixad` when the directory or one + of its top-level entries belongs to another user or group, sets its mode to + `750`, then runs the server as `pixad`; data left by an earlier run under + another uid is taken over this way; "Running under upaas" in `README.md` no + longer tells the operator to create or chown the host directory. - 2026-09-29 variant content types kept in memory (closes #70): `Cache.metaCache` holds the content types of up to 10,000 variants in an LRU (`github.com/hashicorp/golang-lru/v2`), filled by `StoreVariant` and by diff --git a/deploy/docker-entrypoint.sh b/deploy/docker-entrypoint.sh index 691ff2f..abb3f16 100755 --- a/deploy/docker-entrypoint.sh +++ b/deploy/docker-entrypoint.sh @@ -1,14 +1,22 @@ #!/bin/sh # deploy/docker-entrypoint.sh: the Docker image's ENTRYPOINT. It runs as -# root only to give /var/lib/pixa to pixad: a host directory -# bind-mounted there keeps its host owner, often root, and pixad could -# not write to it. The server itself always runs as pixad. +# root only to make /var/lib/pixa usable by pixad: a host directory +# bind-mounted there keeps its host owner, often root, and data from an +# earlier run may belong to another uid. The server itself always runs +# as pixad. set -eu main() { - if [ "$(stat -c %U /var/lib/pixa)" != pixad ]; then - chown pixad:pixad /var/lib/pixa + mkdir -p /var/lib/pixa + # Only the directory and its top-level entries are checked, so a + # normal start does not walk the cache. -depth gives each directory + # to pixad after its contents, so a start stopped part way leaves + # something at the top for the next start to find; -h changes a + # symlink itself, never the file it points to. + if [ -n "$(find /var/lib/pixa -maxdepth 1 \( ! -user pixad -o ! -group pixad \))" ]; then + find /var/lib/pixa -depth -exec chown -h pixad:pixad {} + fi + chmod 750 /var/lib/pixa exec su-exec pixad /usr/local/bin/pixad "$@" } -- 2.54.0