Stop waiting for count writes after a request's deadline (closes #224)

TestService_Get_ReturnsByItsDeadline failed on a busy host because its
request, past its deadline, still waited for its miss count to be
written, a database write with no deadline; in pixad that write waits
for the one database connection every request shares. The hit, miss,
upstream fetch and transform counts are now each written in a goroutine
of their own, which the request waits for only until its deadline; a
count not written by then is written after it returns. Shutdown waits
for those writes within ShutdownTimeout and reports any left unfinished.
The test phase also runs go test with -parallel 4: on a busy host, as
many tests at once as there are CPUs wait so long to be scheduled that
a timed request can fail.

Model: opus-5-5

Dockerfile

@@ -32,10 +32,13 @@ RUN script/bootstrap --cgo
 COPY . .
 
 # Without -v first; on a failure, again with -v for the details, and
-# the step fails even if the second run passes.
+# the step fails even if the second run passes. -parallel 4: by default
-RUN go test -count=1 -timeout 90s -race -cover ./... || \
+# a package runs as many of its tests at once as the host has CPUs, and
+# on a busy host they then wait so long to be scheduled that a test
+# that times a request can see it return a second late.
+RUN go test -count=1 -timeout 90s -race -parallel 4 -cover ./... || \
     { echo "--- Rerunning with -v for details ---"; \
-      go test -count=1 -timeout 90s -race -v ./...; exit 1; }
+      go test -count=1 -timeout 90s -race -parallel 4 -v ./...; exit 1; }
 
 # Build stage. Nothing is wanted from the two phases above: these copies
 # make BuildKit build them first, so this stage runs only when lint and

README.md

@@ -46,9 +46,9 @@ everything in this list without further settings. The reverse proxy must:
   builds encrypted URLs from `Host`, and checks `Referer` against
   `referer_blocklist`;
 - set `X-Forwarded-For` to the client's address, with `trusted_proxies` set to
-  the address pixa sees the proxy's requests come from, so the login limit and
+  the address pixa sees the proxy's requests come from, so the login limit
-  `image_requests_per_minute` count each client by its own address (see
+  counts each client by its own address (see `trusted_proxies` under
-  `trusted_proxies` under Configuration);
+  Configuration);
 - wait for pixa's answer for at least `downstream_timeout` (default `60s`), the
   longest pixa takes to fetch, convert and send an image.
 
@@ -58,9 +58,8 @@ password there.
 
 pixa does the rest itself: it checks signatures and encrypted URLs, applies the
 allowlist, refuses upstream hosts with private or local addresses, limits login
-attempts, each client's image requests, upstream response size and image
+attempts, upstream response size and image dimensions, and sends the security
-dimensions, and sends the security headers, `Strict-Transport-Security`
+headers, `Strict-Transport-Security` included, with every response.
-included, with every response.
 
 The state directory (`state_dir`, `/var/lib/pixa` in the container) holds the
 database and the disk cache:
@@ -84,10 +83,12 @@ which answers 200 whenever pixa is running, in maintenance mode too (see
 `maintenance_mode`).
 
 On SIGTERM or SIGINT pixa stops accepting connections, gives the requests in
-progress and the images being processed 5 seconds to finish, and exits: with 0,
+progress, the images being processed and the counts of cache hits, misses,
-or with 1 when images were still being processed after those 5 seconds or
+fetches and conversions being written to the database 5 seconds to finish, and
-another part of pixa failed to stop. A request not finished by then is cut off.
+exits: with 0, or with 1 when images were still being processed or counts still
-`docker stop` waits 10 seconds before it kills the container.
+being written after those 5 seconds, or another part of pixa failed to stop. A
+request not finished by then is cut off. `docker stop` waits 10 seconds before
+it kills the container.
 
 Outside Docker, pixa needs libvips (the image has 8.16) and libheif to run, as
 it uses libvips through CGO. pixad does not start unless libvips has its JPEG XL
@@ -187,10 +188,10 @@ path under `/v1/` answers 200, in maintenance mode too.
   `Referer` names a host in `referer_blocklist`, checked before the signature,
   the cache and the upstream fetch; 403 when the upstream host, or a host it
   redirects to, is `localhost`, ends in `.localhost` or `.local`, or has an
-  address in a blocked network (see `blocked_networks`); 429 over
+  address in a blocked network (see `blocked_networks`); 502 when the upstream
-  `image_requests_per_minute` (below); 502 when the upstream answered with an
+  answered with an error status, and for 5 minutes after that for the same
-  error status, and for 5 minutes after that for the same source URL; 503 when
+  source URL; 503 when pixa is busy or in maintenance mode; 500 for any other
-  pixa is busy or in maintenance mode; 500 for any other failure.
+  failure.
 - `GET` or `HEAD` `/v1/e/<token>/<name>` — an image through an encrypted URL
   (see Encrypted URLs). Needs: nothing but the URL. Answers: 200; 304 when
   `If-None-Match` matches the image's `ETag`; 400 for a token that does not
@@ -198,7 +199,7 @@ path under `/v1/` answers 200, in maintenance mode too.
   expired; 504 when the upstream has not sent its response headers within
   `upstream_fetch_timeout`, but 500 when that time runs out while the image
   itself is still arriving; 400 for an `Accept` header that is not valid, and
-  406, 403, 429, 502, 503 and 500, as for `/v1/image/`.
+  406, 403, 502, 503 and 500, as for `/v1/image/`.
 - `GET /robots.txt` — asks every crawler to stay away (`Disallow: /`). Needs:
   nothing. Answers: 200.
 - `GET /.well-known/healthcheck.json` — JSON with `status` (`ok`), `now`,
@@ -314,14 +315,6 @@ 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.
 
-One client may send `image_requests_per_minute` requests per minute
-(default 600) to the image routes, `/v1/image/` and `/v1/e/` together, `GET` and
-`HEAD`, cache hits included; a request over the limit is refused with 429 and a
-`Retry-After` header. Clients are counted as for the login limit. A browser's
-CORS preflight is not counted. Behind a CDN or proxy whose addresses are not in
-`trusted_proxies`, every request counts as the same client, so such a deployment
-needs a higher limit, or `0`, which turns the limit off.
-
 ### Encrypted URLs
 
 An encrypted URL is an image URL made on pixa's own web page by someone who
@@ -491,7 +484,6 @@ tell. With no file, pixa uses the environment and the defaults.
 | `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_IMAGE_REQUESTS_PER_MINUTE`     | `image_requests_per_minute`     | Image requests per client per minute; `0` turns it off; default `600`        |
 | `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`                 |
@@ -541,13 +533,13 @@ Key settings in more detail:
   explicit list replaces the default. An invalid CIDR aborts 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 and `image_requests_per_minute` under Routes). For a
+  (see the login limit under Routes). For a proxy on the Docker host that
-  proxy on the Docker host that connects to pixa over `127.0.0.1`, that address
+  connects to pixa over `127.0.0.1`, that address is the gateway of the
-  is the gateway of the container's Docker network (`172.17.0.1` on the default
+  container's Docker network (`172.17.0.1` on the default bridge), not the
-  bridge), not the proxy's own address; a proxy that connects through another of
+  proxy's own address; a proxy that connects through another of the host's
-  the host's addresses is seen with that address. To be sure which address it
+  addresses is seen with that address. To be sure which address it is, set this
-  is, set this to `[]` (or `PIXA_TRUSTED_PROXIES` to empty), send a request
+  to `[]` (or `PIXA_TRUSTED_PROXIES` to empty), send a request through the
-  through the proxy, and read `remoteIP` in pixa's request log line for it
+  proxy, and read `remoteIP` in pixa's request log line for it
 - `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;
@@ -580,11 +572,6 @@ 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
-- `image_requests_per_minute` — how many requests one client may send to the
-  image routes per minute (see the limit on them under Routes); default `600`.
-  `0` turns the limit off, and a negative or non-integer value aborts startup.
-  Behind a CDN or proxy whose addresses are not in `trusted_proxies`, every
-  request counts as the same client, so raise the limit there, or set `0`
 - `maintenance_mode` — while `true`, the image routes (`/v1/image/` and
   `/v1/e/`) answer every request for an image with 503, a `Retry-After` header
   and a JSON error body. The health check (`/.well-known/healthcheck.json`)

TODO.md

@@ -26,18 +26,25 @@ resolved. The disk cache is now size-bounded with LRU eviction
 
 # Next Step
 
-P2: security: per-origin rate limiting
+P2: security: per-IP rate limiting on the image routes
 
 # Completed Steps
 
-- 2026-10-12 per-client limit on the image routes (closes #236): the new setting
+- 2026-10-08 a request past its deadline no longer waits for a database write
-  `image_requests_per_minute` (`PIXA_IMAGE_REQUESTS_PER_MINUTE`), default 600,
+  (closes #224). On a busy host, `TestService_Get_ReturnsByItsDeadline` failed
-  is how many `GET` and `HEAD` requests one client may send to `/v1/image/` and
+  because its request returned over a second after its deadline: it was still
-  `/v1/e/` together per minute, cache hits included; the next is answered 429
+  waiting for its miss count to be written to the database, a write with no
-  with `Retry-After`. It uses the login limit's `RateLimit` middleware, so
+  deadline, which in pixad waits for the one database connection that every
-  clients are told apart the same way, and runs after the CORS middleware, so a
+  request shares. The hit, miss, upstream fetch and transform counts are now
-  preflight is not counted. `0` turns the limit off; a negative or non-integer
+  each written in a goroutine of their own, which the request waits for only
-  value aborts startup.
+  until its deadline; a count not written by then is written after the request
+  returns. On shutdown pixad waits for those writes within the same 5 seconds as
+  for the images being processed, and exits with 1 when some are unfinished. The
+  test phase of the `Dockerfile` also runs at most 4 tests of a package at once
+  (`go test -parallel 4`), where it ran as many as the host has CPUs: on a busy
+  host they then wait so long to be scheduled that a test that times a request
+  can fail. `-p`, how many packages are tested at once, is unchanged, as capping
+  it also slows compiling.
 - 2026-10-08 every output format is saved with settings pixa sets on purpose
   (closes #232): each format has its own govips export, as JPEG XL does, in
   place of govips' generic `Export`, which sent libvips a zero for some settings
@@ -744,6 +751,8 @@ P2: security: per-origin rate limiting
 
 # Future Steps
 
+- P2: security
+    - per-origin rate limiting
 - P2: HTTP response handling
     - Last-Modified headers
 - P2: configuration

configs/config.example.yml

@@ -115,14 +115,6 @@ upstream_connections: 64
 # frees up it is answered 503, unless downstream_timeout has ended first.
 # max_concurrent_processing: 4
 
-# How many requests to the image routes (/v1/image/ and /v1/e/) one client
-# may send per minute; a request over the limit is answered 429 with a
-# Retry-After header. 0 turns the limit off. Clients are told apart as for
-# the login limit, so behind a CDN or proxy whose addresses are not in
-# trusted_proxies every request counts as the same client: raise the limit
-# there, or set 0. (default: 600)
-image_requests_per_minute: 600
-
 # Time allowed for one fetch from an upstream host (default: 30s)
 upstream_fetch_timeout: 30s
 

internal/config/config.go

@@ -34,7 +34,6 @@ const (
 	DefaultUpstreamFetchTimeout       = 30 * time.Second
 	DefaultUpstreamMaxResponseSize    = 50 << 20 // 50 MiB
 	DefaultDownstreamTimeout          = 60 * time.Second
-	DefaultImageRequestsPerMinute     = 600
 )
 
 // Configuration key names.
@@ -62,7 +61,6 @@ const (
 	keyUpstreamFetchTimeout       = "upstream_fetch_timeout"
 	keyUpstreamMaxResponseSize    = "upstream_max_response_size"
 	keyDownstreamTimeout          = "downstream_timeout"
-	keyImageRequestsPerMinute     = "image_requests_per_minute"
 )
 
 // placeholderSigningKey is the dummy signing_key shipped in
@@ -160,11 +158,6 @@ type Config struct {
 	// of the per-request timeout middleware.
 	DownstreamTimeout time.Duration
 
-	// ImageRequestsPerMinute is how many requests one client may make to
-	// the image routes per minute; the next is refused with 429 Too Many
-	// Requests. 0 turns the limit off.
-	ImageRequestsPerMinute int
-
 	// 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.
@@ -309,8 +302,6 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) {
 			keyAccessControlAllowOrigin, DefaultAccessControlAllowOrigin),
 		DownstreamTimeout: loader.durationVal(
 			keyDownstreamTimeout, DefaultDownstreamTimeout),
-		ImageRequestsPerMinute: loader.intVal(
-			keyImageRequestsPerMinute, DefaultImageRequestsPerMinute),
 		CacheMaxBytes:    loader.int64Val(keyCacheMaxBytes, 0),
 		BlockedNetworks:  blockedNetworks,
 		TrustedProxies:   trustedProxies,
@@ -320,7 +311,9 @@ func newFromSmartConfig(sc *smartconfig.Config) (*Config, error) {
 	// The default for an omitted cache_max_bytes is worked out when
 	// the cache opens; here we only record whether the operator set
 	// the key explicitly.
-	_, c.CacheMaxBytesExplicit = lookupValue(sc, keyCacheMaxBytes)
+	if _, present := lookupValue(sc, keyCacheMaxBytes); present {
+		c.CacheMaxBytesExplicit = true
+	}
 
 	// Build DBURL from StateDir if not explicitly set. The derived URL
 	// is a default: it applies only when db_url is omitted, never to an
@@ -433,7 +426,7 @@ func isKnownConfigKey(key string) bool {
 		keyMaxConcurrentProcessing, keyCacheMaxBytes, keyBlockedNetworks,
 		keyTrustedProxies, keyAccessControlAllowOrigin, keyUpstreamFetchTimeout,
 		keyUpstreamMaxResponseSize, keyDownstreamTimeout, keyRefererBlocklist,
-		keyImageRequestsPerMinute, "env":
+		"env":
 		return true
 	}
 
@@ -468,7 +461,6 @@ func envVarNames() map[string]string {
 		keyUpstreamFetchTimeout:       "PIXA_UPSTREAM_FETCH_TIMEOUT",
 		keyUpstreamMaxResponseSize:    "PIXA_UPSTREAM_MAX_RESPONSE_SIZE",
 		keyDownstreamTimeout:          "PIXA_DOWNSTREAM_TIMEOUT",
-		keyImageRequestsPerMinute:     "PIXA_IMAGE_REQUESTS_PER_MINUTE",
 	}
 }
 
@@ -621,14 +613,6 @@ func (c *Config) validate() error {
 			settingName(keyCacheMaxBytes), c.CacheMaxBytes, errMustNotBeNegative)
 	}
 
-	// Zero is valid (it turns the limit off); only negative values are
-	// rejected.
-	if c.ImageRequestsPerMinute < 0 {
-		return fmt.Errorf("%s: value %d %w",
-			settingName(keyImageRequestsPerMinute), c.ImageRequestsPerMinute,
-			errMustNotBeNegative)
-	}
-
 	err = c.validateUpstreamMaxResponseSize()
 	if err != nil {
 		return err
@@ -641,9 +625,12 @@ func (c *Config) validate() error {
 		}
 	}
 
-	err = c.validateSentryDSN()
+	if c.SentryDSN != "" {
-	if err != nil {
+		parsed, err := url.Parse(c.SentryDSN)
-		return err
+		if err != nil || parsed.Scheme == "" || parsed.Host == "" {
+			return fmt.Errorf("%s: value %q is %w",
+				settingName(keySentryDSN), c.SentryDSN, errNotAValidURL)
+		}
 	}
 
 	if (c.MetricsUsername == "") != (c.MetricsPassword == "") {
@@ -655,22 +642,6 @@ func (c *Config) validate() error {
 	return c.validateAccessControlAllowOrigin()
 }
 
-// validateSentryDSN checks that sentry_dsn, when set, is a URL with a scheme
-// and a host.
-func (c *Config) validateSentryDSN() error {
-	if c.SentryDSN == "" {
-		return nil
-	}
-
-	parsed, err := url.Parse(c.SentryDSN)
-	if err != nil || parsed.Scheme == "" || parsed.Host == "" {
-		return fmt.Errorf("%s: value %q is %w",
-			settingName(keySentryDSN), c.SentryDSN, errNotAValidURL)
-	}
-
-	return nil
-}
-
 // 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

internal/config/env_internal_test.go

@@ -70,7 +70,6 @@ func TestEnvironmentSetsEveryKey(t *testing.T) {
 	t.Setenv("PIXA_UPSTREAM_CONNECTIONS_PER_HOST", "5")
 	t.Setenv("PIXA_UPSTREAM_CONNECTIONS", "10")
 	t.Setenv("PIXA_MAX_CONCURRENT_PROCESSING", "3")
-	t.Setenv("PIXA_IMAGE_REQUESTS_PER_MINUTE", "30")
 	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")
@@ -100,7 +99,6 @@ func TestEnvironmentSetsEveryKey(t *testing.T) {
 		UpstreamConnectionsPerHost: 5,
 		UpstreamConnections:        10,
 		MaxConcurrentProcessing:    3,
-		ImageRequestsPerMinute:     30,
 		CacheMaxBytes:              1024,
 		CacheMaxBytesExplicit:      true,
 		BlockedNetworks:            []netip.Prefix{netip.MustParsePrefix("203.0.113.0/24")},

internal/config/image_requests_per_minute_internal_test.go

@@ -1,107 +0,0 @@
-package config
-
-import "testing"
-
-// testImageRequestsPerMinuteVar is the variable that sets
-// image_requests_per_minute.
-const testImageRequestsPerMinuteVar = "PIXA_IMAGE_REQUESTS_PER_MINUTE"
-
-// TestImageRequestsPerMinuteValues checks that an omitted
-// image_requests_per_minute is 600, and that 0, which turns the limit off,
-// and any larger whole number are used as given.
-func TestImageRequestsPerMinuteValues(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.ImageRequestsPerMinute != 600 {
-		t.Errorf("ImageRequestsPerMinute = %d, want 600", c.ImageRequestsPerMinute)
-	}
-
-	explicit := []struct {
-		value string
-		want  int
-	}{
-		{"0", 0},
-		{"30", 30},
-	}
-
-	for _, tc := range explicit {
-		c, err := configFromYAML(t,
-			signingKeyLine+"image_requests_per_minute: "+tc.value+"\n")
-		if err != nil {
-			t.Fatalf("image_requests_per_minute: %s should load, got error: %v",
-				tc.value, err)
-		}
-
-		if c.ImageRequestsPerMinute != tc.want {
-			t.Errorf("ImageRequestsPerMinute = %d, want %d",
-				c.ImageRequestsPerMinute, tc.want)
-		}
-	}
-}
-
-// TestInvalidImageRequestsPerMinuteAbortsStartup checks that a value that is
-// negative, not a whole number, or null aborts startup naming the key and
-// the value, and the variable too where the value could have come from it.
-func TestInvalidImageRequestsPerMinuteAbortsStartup(t *testing.T) {
-	t.Parallel()
-
-	key := keyImageRequestsPerMinute
-
-	runAbortCases(t, []abortCase{
-		{
-			name: "negative",
-			yaml: signingKeyLine + key + ": -1\n",
-			wantErrSubstrings: []string{
-				key, testImageRequestsPerMinuteVar, "value -1",
-			},
-		},
-		{
-			name: "not a number",
-			yaml: signingKeyLine + key + ": plenty\n",
-			wantErrSubstrings: []string{
-				key, testImageRequestsPerMinuteVar, "plenty",
-			},
-		},
-		{
-			name:              "fractional",
-			yaml:              signingKeyLine + key + ": 2.5\n",
-			wantErrSubstrings: []string{key, "2.5"},
-		},
-		{
-			name:              "explicit null",
-			yaml:              signingKeyLine + key + ": null\n",
-			wantErrSubstrings: []string{key, nullValueText},
-		},
-	})
-}
-
-// TestImageRequestsPerMinuteFromEnvironment checks that the variable sets the
-// limit over the config file, and that an invalid value in it aborts startup
-// naming the variable and the value.
-func TestImageRequestsPerMinuteFromEnvironment(t *testing.T) {
-	t.Setenv(testImageRequestsPerMinuteVar, "0")
-
-	c, err := configFromYAML(t, signingKeyLine+"image_requests_per_minute: 30\n")
-	if err != nil {
-		t.Fatalf("limit from the environment should load: %v", err)
-	}
-
-	if c.ImageRequestsPerMinute != 0 {
-		t.Errorf("ImageRequestsPerMinute = %d, want 0 from the environment",
-			c.ImageRequestsPerMinute)
-	}
-
-	for _, value := range []string{"-5", "ten"} {
-		t.Run(value, func(t *testing.T) {
-			t.Setenv(testImageRequestsPerMinuteVar, value)
-
-			_, err := configFromYAML(t, signingKeyLine)
-			wantStartupError(t, err, testImageRequestsPerMinuteVar, value)
-		})
-	}
-}

internal/handlers/handlers.go

@@ -93,6 +93,13 @@ func (s *Handlers) WaitForProcessing(ctx context.Context) int {
 	return s.imgSvc.WaitForProcessing(ctx)
 }
 
+// WaitForCountWrites waits until every count a request has started writing
+// to the database is written, or until ctx ends, and reports whether they
+// all were.
+func (s *Handlers) WaitForCountWrites(ctx context.Context) bool {
+	return s.imgSvc.WaitForCountWrites(ctx)
+}
+
 // newCacheConfig builds the image cache's configuration from cfg.
 // cache_max_bytes: 0 disables the disk cache entirely; any other value
 // is the eviction limit in bytes; when it is omitted, the cache works

internal/imgcache/count_writes_internal_test.go

@@ -0,0 +1,116 @@
+package imgcache
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+)
+
+// holdDatabase takes the one connection of the test service's database, so
+// that every other query waits for it, and returns the func that frees it.
+func holdDatabase(t *testing.T, svc *Service) func() {
+	t.Helper()
+
+	conn, err := svc.cache.db.Conn(t.Context())
+	if err != nil {
+		t.Fatalf("failed to take the database connection: %v", err)
+	}
+
+	release := func() { _ = conn.Close() }
+	t.Cleanup(release)
+
+	return release
+}
+
+// TestService_Get_ReturnsByItsDeadlineWhileTheDatabaseIsBusy holds the
+// database's one connection while a request whose fetch is held reaches its
+// deadline. The request must still return by its deadline with the
+// deadline's error, and its miss must be counted once the connection is free.
+func TestService_Get_ReturnsByItsDeadlineWhileTheDatabaseIsBusy(t *testing.T) {
+	t.Parallel()
+
+	svc, fixtures, fetcher := setupHeldFetchService(t)
+
+	const timeout = 200 * time.Millisecond
+
+	ctx, cancel := context.WithTimeout(t.Context(), timeout)
+	defer cancel()
+
+	results := startGet(ctx, svc, photoVariant(fixtures, 85, FitCover))
+
+	// The request has made its database reads by the time it fetches.
+	<-fetcher.started
+
+	releaseDatabase := holdDatabase(t, svc)
+
+	select {
+	case got := <-results:
+		deadline, _ := ctx.Deadline()
+		t.Logf("Get() returned %v after its deadline, error = %v",
+			time.Since(deadline), got.err)
+
+		if !errors.Is(got.err, context.DeadlineExceeded) {
+			t.Errorf("Get() error = %v, want %v", got.err, context.DeadlineExceeded)
+		}
+	case <-time.After(timeout + time.Second):
+		t.Fatal("request did not return by its deadline while the database was busy")
+	}
+
+	releaseDatabase()
+
+	waitCtx, cancelWait := context.WithTimeout(t.Context(), 5*time.Second)
+	defer cancelWait()
+
+	if !svc.WaitForCountWrites(waitCtx) {
+		t.Fatal("the miss was not counted once the database was free")
+	}
+
+	want := cacheStatsCounters{missCount: 1}
+	if got := readCacheStatsCounters(t, svc.cache); got != want {
+		t.Errorf("counters = %+v, want %+v", got, want)
+	}
+}
+
+// TestService_WaitForCountWrites holds the database's one connection while a
+// request past its deadline counts a miss. WaitForCountWrites must report the
+// count unwritten when its context ends, and written once the connection is
+// free.
+func TestService_WaitForCountWrites(t *testing.T) {
+	t.Parallel()
+
+	svc, _, _ := setupHeldFetchService(t)
+
+	releaseDatabase := holdDatabase(t, svc)
+
+	ended, cancel := context.WithDeadline(t.Context(), time.Now())
+	defer cancel()
+
+	svc.writeCount(ended, func(writeCtx context.Context) {
+		svc.cache.IncrementStats(writeCtx, false, 0)
+	})
+
+	shortCtx, cancelShort := context.WithTimeout(t.Context(), 100*time.Millisecond)
+	defer cancelShort()
+
+	written := svc.WaitForCountWrites(shortCtx)
+	t.Logf("WaitForCountWrites() while the database was busy = %t", written)
+
+	if written {
+		t.Fatal("WaitForCountWrites() = true while the database was busy")
+	}
+
+	releaseDatabase()
+
+	waitCtx, cancelWait := context.WithTimeout(t.Context(), 5*time.Second)
+	defer cancelWait()
+
+	if !svc.WaitForCountWrites(waitCtx) {
+		t.Fatal("WaitForCountWrites() = false once the database was free")
+	}
+
+	want := cacheStatsCounters{missCount: 1}
+	if got := readCacheStatsCounters(t, svc.cache); got != want {
+		t.Errorf("counters = %+v, want %+v", got, want)
+	}
+}

internal/imgcache/service.go

@@ -9,6 +9,7 @@ import (
 	"log/slog"
 	"net/url"
 	"runtime/debug"
+	"sync"
 	"time"
 
 	"github.com/dustin/go-humanize"
@@ -36,6 +37,9 @@ type Service struct {
 	// variantsInProgress lets the requests that miss the same variant at the
 	// same time share one fetch and one transcode.
 	variantsInProgress singleflight.Group
+	// countWrites holds the count writes writeCount has started, for
+	// WaitForCountWrites.
+	countWrites sync.WaitGroup
 }
 
 // ServiceConfig holds configuration for the image service.
@@ -161,8 +165,9 @@ 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 {
-			// Counted also when the request context has ended meanwhile
+			s.writeCount(ctx, func(writeCtx context.Context) {
-			s.cache.IncrementStats(context.WithoutCancel(ctx), true, 0)
+				s.cache.IncrementStats(writeCtx, true, 0)
+			})
 
 			return &ImageResponse{
 				Content:       reader,
@@ -179,7 +184,9 @@ func (s *Service) Get(ctx context.Context, req *ImageRequest) (*ImageResponse, e
 	// failed or the request context has ended meanwhile
 	response, err := s.processOrWait(ctx, req)
 
-	s.cache.IncrementStats(context.WithoutCancel(ctx), false, 0)
+	s.writeCount(ctx, func(writeCtx context.Context) {
+		s.cache.IncrementStats(writeCtx, false, 0)
+	})
 
 	if err != nil {
 		return nil, err
@@ -208,6 +215,53 @@ func (s *Service) WaitForProcessing(ctx context.Context) int {
 	return s.processor.WaitForProcessing(ctx)
 }
 
+// WaitForCountWrites waits until every count a request has started writing
+// to the database is written, or until ctx ends, and reports whether they
+// all were.
+func (s *Service) WaitForCountWrites(ctx context.Context) bool {
+	written := make(chan struct{})
+
+	go func() {
+		s.countWrites.Wait()
+		close(written)
+	}()
+
+	select {
+	case <-written:
+		return true
+	case <-ctx.Done():
+		return false
+	}
+}
+
+// writeCount runs write, which adds to a count in the database, in a
+// goroutine of its own, and waits for it, though not past ctx's deadline.
+// The write gets ctx without its cancellation or deadline, so an ended
+// request is still counted. A request past its deadline thus does not wait
+// for the database connection, which every request shares; its count is
+// written after it returns.
+func (s *Service) writeCount(ctx context.Context, write func(context.Context)) {
+	written := make(chan struct{})
+
+	s.countWrites.Go(func() {
+		defer close(written)
+
+		write(context.WithoutCancel(ctx))
+	})
+
+	deadline, hasDeadline := ctx.Deadline()
+	if !hasDeadline {
+		<-written
+
+		return
+	}
+
+	select {
+	case <-written:
+	case <-time.After(time.Until(deadline)):
+	}
+}
+
 // ValidateRequest validates the request signature if required.
 func (s *Service) ValidateRequest(req *ImageRequest) error {
 	// Check if host is allowed (no signature required)
@@ -452,8 +506,9 @@ func (s *Service) fetchAndProcess(
 	sourceData, err := io.ReadAll(fetchResult.Content)
 	fetchBytes := int64(len(sourceData))
 
-	// Counted also when the request context has ended meanwhile
+	s.writeCount(ctx, func(writeCtx context.Context) {
-	s.cache.IncrementUpstreamFetch(context.WithoutCancel(ctx), fetchBytes)
+		s.cache.IncrementUpstreamFetch(writeCtx, fetchBytes)
+	})
 
 	if err != nil {
 		return nil, fmt.Errorf("failed to read upstream response: %w", err)
@@ -534,8 +589,7 @@ func (s *Service) processAndStore(
 
 	processDuration := time.Since(processStart)
 
-	// Counted also when the request context has ended meanwhile
+	s.writeCount(ctx, s.cache.IncrementTransformCount)
-	s.cache.IncrementTransformCount(context.WithoutCancel(ctx))
 
 	// Read processed content
 	processedData, err := io.ReadAll(processResult.Content)

internal/server/image_rate_limit_internal_test.go

@@ -1,152 +0,0 @@
-package server
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"strconv"
-	"testing"
-
-	"sneak.berlin/go/pixa/internal/config"
-)
-
-// requestImageFrom sends method path to s from remoteAddr and returns the
-// response. The image handlers refuse an unsigned image URL with 401 and a
-// token they cannot decrypt with 400, so either status shows the limit let
-// the request through.
-func requestImageFrom(
-	t *testing.T, s *Server, method, path, remoteAddr string,
-) *httptest.ResponseRecorder {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(t.Context(), method, path, nil)
-	req.RemoteAddr = remoteAddr
-
-	rec := httptest.NewRecorder()
-	s.ServeHTTP(rec, req)
-
-	return rec
-}
-
-// TestImageRequestLimitRefusesRequestOverLimit verifies that GET and HEAD on
-// both image routes count toward one limit per client, and that the request
-// after image_requests_per_minute of them is refused with 429 and a
-// Retry-After header.
-func TestImageRequestLimitRefusesRequestOverLimit(t *testing.T) {
-	t.Parallel()
-
-	requests := []struct {
-		method string
-		path   string
-		want   int
-	}{
-		{http.MethodGet, unsignedImagePath, http.StatusUnauthorized},
-		{http.MethodHead, unsignedImagePath, http.StatusUnauthorized},
-		{http.MethodGet, encryptedImagePath, http.StatusBadRequest},
-	}
-
-	s := newTestServer(t)
-	s.config.ImageRequestsPerMinute = len(requests)
-	s.SetupRoutes()
-
-	for _, tc := range requests {
-		rec := requestImageFrom(t, s, tc.method, tc.path, firstClient)
-		if rec.Code != tc.want {
-			t.Fatalf("%s %s within the limit status = %d, want %d",
-				tc.method, tc.path, rec.Code, tc.want)
-		}
-	}
-
-	rec := requestImageFrom(t, s, http.MethodHead, encryptedImagePath, firstClient)
-	if rec.Code != http.StatusTooManyRequests {
-		t.Fatalf("request over the limit status = %d, want %d",
-			rec.Code, http.StatusTooManyRequests)
-	}
-
-	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)
-	}
-}
-
-// TestImageRequestLimitCountsClientsSeparately verifies that one client going
-// over the limit does not limit another.
-func TestImageRequestLimitCountsClientsSeparately(t *testing.T) {
-	t.Parallel()
-
-	s := newTestServer(t)
-	s.config.ImageRequestsPerMinute = 1
-	s.SetupRoutes()
-
-	rec := requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
-	if rec.Code != http.StatusUnauthorized {
-		t.Fatalf("first client's first request status = %d, want %d",
-			rec.Code, http.StatusUnauthorized)
-	}
-
-	rec = requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
-	if rec.Code != http.StatusTooManyRequests {
-		t.Fatalf("first client's second request status = %d, want %d",
-			rec.Code, http.StatusTooManyRequests)
-	}
-
-	rec = requestImageFrom(t, s, http.MethodGet, unsignedImagePath, secondClient)
-	if rec.Code != http.StatusUnauthorized {
-		t.Errorf("second client's first request status = %d, want %d",
-			rec.Code, http.StatusUnauthorized)
-	}
-}
-
-// TestImageRequestLimitZeroLetsEveryRequestThrough verifies that with
-// image_requests_per_minute set to 0 no image request is refused with 429,
-// not even past the default limit.
-func TestImageRequestLimitZeroLetsEveryRequestThrough(t *testing.T) {
-	t.Parallel()
-
-	s := newTestServer(t)
-	s.config.ImageRequestsPerMinute = 0
-	s.SetupRoutes()
-
-	for request := range config.DefaultImageRequestsPerMinute + 1 {
-		rec := requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
-		if rec.Code != http.StatusUnauthorized {
-			t.Fatalf("request %d status = %d, want %d",
-				request+1, rec.Code, http.StatusUnauthorized)
-		}
-	}
-}
-
-// TestImageRequestLimitDoesNotCountPreflight verifies that a browser's CORS
-// preflight request to an image route is answered without being counted, so
-// a client that has sent several still gets its image request through.
-func TestImageRequestLimitDoesNotCountPreflight(t *testing.T) {
-	t.Parallel()
-
-	s := newTestServer(t)
-	s.config.AccessControlAllowOrigin = config.DefaultAccessControlAllowOrigin
-	s.config.ImageRequestsPerMinute = 1
-	s.SetupRoutes()
-
-	for range 3 {
-		req := httptest.NewRequestWithContext(
-			t.Context(), http.MethodOptions, unsignedImagePath, nil)
-		req.RemoteAddr = firstClient
-		req.Header.Set("Origin", "https://app.example.com")
-		req.Header.Set("Access-Control-Request-Method", http.MethodGet)
-
-		rec := httptest.NewRecorder()
-		s.ServeHTTP(rec, req)
-
-		if rec.Code != http.StatusOK {
-			t.Fatalf("preflight status = %d, want %d", rec.Code, http.StatusOK)
-		}
-	}
-
-	rec := requestImageFrom(t, s, http.MethodGet, unsignedImagePath, firstClient)
-	if rec.Code != http.StatusUnauthorized {
-		t.Errorf("image request after preflights status = %d, want %d",
-			rec.Code, http.StatusUnauthorized)
-	}
-}

internal/server/routes.go

@@ -88,13 +88,6 @@ func (s *Server) SetupRoutes() {
 		r.Group(func(r chi.Router) {
 			r.Use(s.refuseDuringMaintenance)
 
-			// Limited per client, as the login POST is; 0 turns the limit
-			// off. The CORS middleware answers a preflight before it gets
-			// here, so a preflight is not counted.
-			if s.config.ImageRequestsPerMinute > 0 {
-				r.Use(s.mw.RateLimit(s.config.ImageRequestsPerMinute, time.Minute))
-			}
-
 			// Main image proxy route
 			// /v1/image/<host>/<path>/<width>x<height>.<format>, or with no
 			// format /v1/image/<host>/<path>/<width>x<height>

internal/server/server.go

@@ -29,6 +29,10 @@ const (
 // still being processed once ShutdownTimeout has passed.
 var errStillProcessing = errors.New("images still being processed at shutdown")
 
+// errStillWritingCounts is returned by the server's stop hook when counts
+// are still being written to the database once ShutdownTimeout has passed.
+var errStillWritingCounts = errors.New("counts still being written at shutdown")
+
 // Params defines dependencies for Server.
 type Params struct {
 	fx.In
@@ -117,9 +121,11 @@ func (s *Server) enableSentry() error {
 }
 
 // cleanShutdown stops the HTTP server, waits for the images still being
-// processed, then flushes Sentry. The first two share ShutdownTimeout. It
+// processed and then for the counts still being written to the database,
-// returns errStillProcessing when images are still being processed after
+// which closes after this hook, then flushes Sentry. The first three share
-// that, as their work is abandoned.
+// ShutdownTimeout. It returns errStillProcessing and errStillWritingCounts
+// for the images and counts still unfinished after that, as their work is
+// abandoned.
 func (s *Server) cleanShutdown(ctx context.Context) error {
 	s.log.Info("shutting down")
 
@@ -132,17 +138,26 @@ func (s *Server) cleanShutdown(ctx context.Context) error {
 	}
 
 	stillProcessing := s.h.WaitForProcessing(ctxShutdown)
+	countsWritten := s.h.WaitForCountWrites(ctxShutdown)
 
 	if s.sentryEnabled {
 		sentry.Flush(SentryFlushTimeout)
 	}
 
+	var unfinished []error
+
 	if stillProcessing > 0 {
 		s.log.Error("images still being processed at shutdown",
 			"count", stillProcessing)
 
-		return errStillProcessing
+		unfinished = append(unfinished, errStillProcessing)
 	}
 
-	return nil
+	if !countsWritten {
+		s.log.Error("counts still being written at shutdown")
+
+		unfinished = append(unfinished, errStillWritingCounts)
+	}
+
+	return errors.Join(unfinished...)
 }