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).
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.
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
Plan, checked against next at be060a8: the per-host cap in internal/httpfetcher/httpfetcher.go is still the only limit, and vips.Startup(nil) is at internal/imageprocessor/imageprocessor.go:24.
max_concurrent_processing (PIXA_MAX_CONCURRENT_PROCESSING), default the number of CPUs Go uses (runtime.GOMAXPROCS(0)): decoding and encoding are CPU-bound, so more images at once than CPUs only adds memory; unlike the plain CPU count, this follows a container's CPU limit. The image processor takes its slot before it copies the source bytes, so that copy, the decoded image and the output are all inside the limit.
upstream_connections (PIXA_UPSTREAM_CONNECTIONS), default 64: upstream_connections_per_host counted over all hosts together, about three times its default of 20, so one slow host cannot hold them all. The fetcher takes it after the per-host slot and frees both when the response body is closed, which is after the image is processed, so it also caps the fetched images held in memory.
Wait: 10 seconds, fixed, then 503 "server busy, try again later": long enough to ride out a burst, short enough that waiting on both limits plus the default 30 s fetch still fits the default 60 s request timeout. Each slot is freed by a defer (or the body close) set up right after it is taken.
vips.Startup: ConcurrencyLevel: 1, since the processing limit already runs one image per CPU and more libvips threads per image would oversubscribe them; operation cache off (MaxCacheSize, MaxCacheMem, MaxCacheFiles all 0), since each request decodes different bytes, so the cache would rarely be hit and would hold up to 50 MiB outside the limit.
Both settings are whole numbers of at least 1, parsed strictly. Request coalescing (#65) stays separate.
Model: opus-5-5
Plan, checked against `next` at `be060a8`: the per-host cap in `internal/httpfetcher/httpfetcher.go` is still the only limit, and `vips.Startup(nil)` is at `internal/imageprocessor/imageprocessor.go:24`.
- `max_concurrent_processing` (`PIXA_MAX_CONCURRENT_PROCESSING`), default the number of CPUs Go uses (`runtime.GOMAXPROCS(0)`): decoding and encoding are CPU-bound, so more images at once than CPUs only adds memory; unlike the plain CPU count, this follows a container's CPU limit. The image processor takes its slot before it copies the source bytes, so that copy, the decoded image and the output are all inside the limit.
- `upstream_connections` (`PIXA_UPSTREAM_CONNECTIONS`), default `64`: `upstream_connections_per_host` counted over all hosts together, about three times its default of 20, so one slow host cannot hold them all. The fetcher takes it after the per-host slot and frees both when the response body is closed, which is after the image is processed, so it also caps the fetched images held in memory.
- Wait: 10 seconds, fixed, then 503 "server busy, try again later": long enough to ride out a burst, short enough that waiting on both limits plus the default 30 s fetch still fits the default 60 s request timeout. Each slot is freed by a `defer` (or the body close) set up right after it is taken.
- `vips.Startup`: `ConcurrencyLevel: 1`, since the processing limit already runs one image per CPU and more libvips threads per image would oversubscribe them; operation cache off (`MaxCacheSize`, `MaxCacheMem`, `MaxCacheFiles` all `0`), since each request decodes different bytes, so the cache would rarely be hit and would hold up to 50 MiB outside the limit.
Both settings are whole numbers of at least 1, parsed strictly. Request coalescing (https://git.eeqj.de/sneak/pixa/issues/65) stays separate.
Model: opus-5-5
Built as planned above in #148 (branch issue-64-concurrency-limits); no departures from the plan.
Model: opus-5-5
Built as planned above in https://git.eeqj.de/sneak/pixa/pulls/148 (branch `issue-64-concurrency-limits`); no departures from the plan.
Model: opus-5-5
Question for sneak: approve one edit to an existing test in #148, and if you agree, the same kind of edit from now on?
TestEnvironmentSetsEveryKey sets every PIXA_ variable and compares the whole resulting config, so each new setting must add its variable and expected value to it, or the test fails. PR 148 adds two settings (max_concurrent_processing, upstream_connections) and so adds two lines there; no assertion changes. #142 did the same for four settings, which you approved.
Options: (1) approve this edit and, standing, any future edit that only adds a new setting's variable and value to this test (recommended; saves asking you for every new setting); (2) approve this edit only; (3) refuse.
Until you answer, the PR goes through review but is not merged; other work carries on.
Model: opus-5-5
Question for sneak: approve one edit to an existing test in https://git.eeqj.de/sneak/pixa/pulls/148, and if you agree, the same kind of edit from now on?
`TestEnvironmentSetsEveryKey` sets every `PIXA_` variable and compares the whole resulting config, so each new setting must add its variable and expected value to it, or the test fails. PR 148 adds two settings (`max_concurrent_processing`, `upstream_connections`) and so adds two lines there; no assertion changes. https://git.eeqj.de/sneak/pixa/pulls/142 did the same for four settings, which you approved.
Options: (1) approve this edit and, standing, any future edit that only adds a new setting's variable and value to this test (recommended; saves asking you for every new setting); (2) approve this edit only; (3) refuse.
Until you answer, the PR goes through review but is not merged; other work carries on.
Model: opus-5-5
sneak
was assigned by clawbot2026-09-29 03:25:27 +02:00
Standing approval recorded (sneak, #64 (comment), option 1): the two lines #148 adds to TestEnvironmentSetsEveryKey are approved, and from now on any edit to that test that only adds a new setting's variable and its expected value needs no question. Any other change to an existing test still needs the owner's approval.
Model: opus-5-5
Standing approval recorded (sneak, https://git.eeqj.de/sneak/pixa/issues/64#issuecomment-106049, option 1): the two lines https://git.eeqj.de/sneak/pixa/pulls/148 adds to `TestEnvironmentSetsEveryKey` are approved, and from now on any edit to that test that only adds a new setting's variable and its expected value needs no question. Any other change to an existing test still needs the owner's approval.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Verified against
mainat61f42e6. This is the "rate limit global concurrent upstream fetches to prevent resource exhaustion" P1 item fromTODO.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:
MaxResponseSize, 50 MiB) atinternal/imageprocessor/imageprocessor.go:142-151internal/imgcache/service.go:254internal/imgcache/service.go:323With 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
vips.Startupconfiguration (concurrency, operation cache) instead ofnil, with the chosen values explained in the PR.make checkgreen (the suite runs with-raceas of #55).Related
Service.Get.Plan, checked against
nextatbe060a8: the per-host cap ininternal/httpfetcher/httpfetcher.gois still the only limit, andvips.Startup(nil)is atinternal/imageprocessor/imageprocessor.go:24.max_concurrent_processing(PIXA_MAX_CONCURRENT_PROCESSING), default the number of CPUs Go uses (runtime.GOMAXPROCS(0)): decoding and encoding are CPU-bound, so more images at once than CPUs only adds memory; unlike the plain CPU count, this follows a container's CPU limit. The image processor takes its slot before it copies the source bytes, so that copy, the decoded image and the output are all inside the limit.upstream_connections(PIXA_UPSTREAM_CONNECTIONS), default64:upstream_connections_per_hostcounted over all hosts together, about three times its default of 20, so one slow host cannot hold them all. The fetcher takes it after the per-host slot and frees both when the response body is closed, which is after the image is processed, so it also caps the fetched images held in memory.defer(or the body close) set up right after it is taken.vips.Startup:ConcurrencyLevel: 1, since the processing limit already runs one image per CPU and more libvips threads per image would oversubscribe them; operation cache off (MaxCacheSize,MaxCacheMem,MaxCacheFilesall0), since each request decodes different bytes, so the cache would rarely be hit and would hold up to 50 MiB outside the limit.Both settings are whole numbers of at least 1, parsed strictly. Request coalescing (#65) stays separate.
Model: opus-5-5
Built as planned above in #148 (branch
issue-64-concurrency-limits); no departures from the plan.Model: opus-5-5
Question for sneak: approve one edit to an existing test in #148, and if you agree, the same kind of edit from now on?
TestEnvironmentSetsEveryKeysets everyPIXA_variable and compares the whole resulting config, so each new setting must add its variable and expected value to it, or the test fails. PR 148 adds two settings (max_concurrent_processing,upstream_connections) and so adds two lines there; no assertion changes. #142 did the same for four settings, which you approved.Options: (1) approve this edit and, standing, any future edit that only adds a new setting's variable and value to this test (recommended; saves asking you for every new setting); (2) approve this edit only; (3) refuse.
Until you answer, the PR goes through review but is not merged; other work carries on.
Model: opus-5-5
1 yes approved with standing change
Standing approval recorded (sneak, #64 (comment), option 1): the two lines #148 adds to
TestEnvironmentSetsEveryKeyare approved, and from now on any edit to that test that only adds a new setting's variable and its expected value needs no question. Any other change to an existing test still needs the owner's approval.Model: opus-5-5
clawbot referenced this issue2026-09-29 10:32:26 +02:00