P1: no global cap on concurrent image processing — unbounded memory under load #64

New Issue

clawbot · 2026-08-09T03:43:51+02:00

clawbot commented

2026-08-09 03:43:51 +02:00

Verified against main at 61f42e6. This is the "rate limit global concurrent upstream fetches to prevent resource exhaustion" P1 item from TODO.md, but the survey found the processing side is the larger risk, so scoping the issue around both.

There is no global concurrency limiter anywhere in the request path. What exists is MaxConnectionsPerHost, a per-upstream-host semaphore (internal/httpfetcher/httpfetcher.go:154-165), which does nothing to bound total in-flight work across many hosts.

Per cache miss, the following are held in memory simultaneously:

the source bytes, read whole (up to MaxResponseSize, 50 MiB) at internal/imageprocessor/imageprocessor.go:142-151
read again at internal/imgcache/service.go:254
the decoded raster inside libvips (far larger than the encoded source)
the whole processed output, read into memory at internal/imgcache/service.go:323

With N concurrent misses that is N x (source + raster + output) resident, with no ceiling. Against the 1k-5k req/s target in README.md:49, a modest burst of misses across distinct hosts is an OOM, and this is reachable by anyone who can produce valid URLs.

vips.Startup(nil) (imageprocessor.go:22) also accepts libvips' default concurrency and cache settings rather than choosing them deliberately.

Definition of done

A global semaphore bounding concurrent image processing operations, with the limit configurable (strict parsing, abort on invalid — per repo policy) and a sane default derived from CPU count.
A global bound on concurrent upstream fetches, distinct from the existing per-host cap.
Requests that would exceed the limit wait rather than failing, up to a bounded timeout; on timeout return 503 with a clear error, never a silent success or a wrong-format fallback.
Deliberate vips.Startup configuration (concurrency, operation cache) instead of nil, with the chosen values explained in the PR.
Semaphore release is correct on every path including error and early return — a leaked slot permanently reduces capacity. Cover this with a test.
Failing tests first: a test that concurrent processing never exceeds the configured limit, and a test that the slot is released when processing returns an error.
make check green (the suite runs with -race as of #55).

Consider implementing alongside the request-coalescing issue — both bound the same blowup and touch Service.Get.
Interacts with #51/PR #55: eviction bounds disk, this bounds memory and CPU. Both are needed to call the resource-exhaustion class closed.

Verified against `main` at `61f42e6`. This is the "rate limit global concurrent upstream fetches to prevent resource exhaustion" P1 item from `TODO.md`, but the survey found the *processing* side is the larger risk, so scoping the issue around both. There is no global concurrency limiter anywhere in the request path. What exists is `MaxConnectionsPerHost`, a **per-upstream-host** semaphore (`internal/httpfetcher/httpfetcher.go:154-165`), which does nothing to bound total in-flight work across many hosts. Per cache miss, the following are held in memory simultaneously: - the source bytes, read whole (up to `MaxResponseSize`, 50 MiB) at `internal/imageprocessor/imageprocessor.go:142-151` - read again at `internal/imgcache/service.go:254` - the decoded raster inside libvips (far larger than the encoded source) - the whole processed output, read into memory at `internal/imgcache/service.go:323` With N concurrent misses that is N x (source + raster + output) resident, with no ceiling. Against the 1k-5k req/s target in `README.md:49`, a modest burst of misses across distinct hosts is an OOM, and this is reachable by anyone who can produce valid URLs. `vips.Startup(nil)` (`imageprocessor.go:22`) also accepts libvips' default concurrency and cache settings rather than choosing them deliberately. ## Definition of done 1. A global semaphore bounding concurrent image *processing* operations, with the limit configurable (strict parsing, abort on invalid — per repo policy) and a sane default derived from CPU count. 2. A global bound on concurrent upstream fetches, distinct from the existing per-host cap. 3. Requests that would exceed the limit wait rather than failing, up to a bounded timeout; on timeout return 503 with a clear error, never a silent success or a wrong-format fallback. 4. Deliberate `vips.Startup` configuration (concurrency, operation cache) instead of `nil`, with the chosen values explained in the PR. 5. Semaphore release is correct on every path including error and early return — a leaked slot permanently reduces capacity. Cover this with a test. 6. Failing tests first: a test that concurrent processing never exceeds the configured limit, and a test that the slot is released when processing returns an error. 7. `make check` green (the suite runs with `-race` as of #55). ## Related - Consider implementing alongside the request-coalescing issue — both bound the same blowup and touch `Service.Get`. - Interacts with #51/PR #55: eviction bounds *disk*, this bounds *memory and CPU*. Both are needed to call the resource-exhaustion class closed.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:43:51 +02:00

clawbot referenced this issue

2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#64