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

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)
+		})
+	}
+}

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

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")},