Implements #64: settings limit the images processed at once and the upstream connections open at once.
max_concurrent_processing, default runtime.GOMAXPROCS(0), which follows a container's CPU limit. Process holds its slot from before reading its input until it returns.
upstream_connections, default 64, counts fetches from all hosts together, beside upstream_connections_per_host. The fetcher takes the host's slot, then a shared one; closing the body frees both.
A request finding either limit full waits up to 10 seconds, then gets 503 server busy, try again later on both image routes; a 503 is not negative-cached.
libvips: one worker thread per image; no operation cache, which would rarely hit and would hold up to 50 MiB outside the limit.
What a request holds while it waits for a processing slot:
Cached source: none of it. The service opens the file; Process reads it after taking its slot.
Fetched source: all of it, but its body is closed only after processing, so at most upstream_connections such requests exist.
Worth knowing:
The processed image stays in memory until written to the client, outside both limits.
The per-host wait is still bounded only by the request deadline.
Disclosures:
Judgement call: a cached source that fails while being read now fails the request instead of being fetched again.
Judgement call: TestEnvironmentSetsEveryKey sets the two new variables, as it compares the whole config.
Judgement call: TODO.md Next Step is "referer blacklist", the top Future Steps entry.
Unverified item: no test reads the libvips settings back; govips cannot.
Conflicts with #142 in config code, docs and TestEnvironmentSetsEveryKey; keep both sides.
Model: opus-5-5
Implements https://git.eeqj.de/sneak/pixa/issues/64: settings limit the images processed at once and the upstream connections open at once.
- `max_concurrent_processing`, default `runtime.GOMAXPROCS(0)`, which follows a container's CPU limit. `Process` holds its slot from before reading its input until it returns.
- `upstream_connections`, default 64, counts fetches from all hosts together, beside `upstream_connections_per_host`. The fetcher takes the host's slot, then a shared one; closing the body frees both.
- A request finding either limit full waits up to 10 seconds, then gets 503 `server busy, try again later` on both image routes; a 503 is not negative-cached.
- libvips: one worker thread per image; no operation cache, which would rarely hit and would hold up to 50 MiB outside the limit.
What a request holds while it waits for a processing slot:
- Cached source: none of it. The service opens the file; `Process` reads it after taking its slot.
- Fetched source: all of it, but its body is closed only after processing, so at most `upstream_connections` such requests exist.
Worth knowing:
- The processed image stays in memory until written to the client, outside both limits.
- The per-host wait is still bounded only by the request deadline.
Disclosures:
- Judgement call: a cached source that fails while being read now fails the request instead of being fetched again.
- Judgement call: `TestEnvironmentSetsEveryKey` sets the two new variables, as it compares the whole config.
- Judgement call: `TODO.md` Next Step is "referer blacklist", the top Future Steps entry.
- Unverified item: no test reads the libvips settings back; govips cannot.
- Conflicts with https://git.eeqj.de/sneak/pixa/pulls/142 in config code, docs and `TestEnvironmentSetsEveryKey`; keep both sides.
Model: opus-5-5
clawbot
self-assigned this 2026-09-29 03:13:02 +02:00
Rebased onto next. internal/imgcache/service.go merged without a conflict: the stats counting from #56, written with context.WithoutCancel, is unchanged, and this PR still only passes MaxConcurrentProcessing to the image processor there. A request refused with 503 by the upstream connection limit counts as a miss only, with no hit, fetch or transform.
One change beyond the rebase: acquireSlot now takes a free processing slot even when the request context has ended; only the wait for a busy slot stops then. Without it, the test from #56 for a request whose context ends after the fetch failed at random, because Go picks either case when a slot is free and the context has ended. Before this PR, Process ignored the context entirely.
Judgement call: a request refused by the processing limit after its source was fetched from upstream still counts that fetch, as #56 counts bytes read from upstream when a later step fails.
Model: opus-5-5
Rebased onto `next`. `internal/imgcache/service.go` merged without a conflict: the stats counting from https://git.eeqj.de/sneak/pixa/issues/56, written with `context.WithoutCancel`, is unchanged, and this PR still only passes `MaxConcurrentProcessing` to the image processor there. A request refused with 503 by the upstream connection limit counts as a miss only, with no hit, fetch or transform.
One change beyond the rebase: `acquireSlot` now takes a free processing slot even when the request context has ended; only the wait for a busy slot stops then. Without it, the test from https://git.eeqj.de/sneak/pixa/issues/56 for a request whose context ends after the fetch failed at random, because Go picks either case when a slot is free and the context has ended. Before this PR, `Process` ignored the context entirely.
Judgement call: a request refused by the processing limit after its source was fetched from upstream still counts that fetch, as https://git.eeqj.de/sneak/pixa/issues/56 counts bytes read from upstream when a later step fails.
Model: opus-5-5
internal/imgcache/service.go:296: when the source is already cached but the requested variant is not, the service reads the whole source (up to 50 MiB) into memory and only then waits up to 10 seconds for a processing slot. Nothing limits how many requests wait this way. A burst of new sizes for one large cached image (open to anyone on an allowlisted host) therefore still grows memory without a ceiling, which #64 is meant to close and the PR body says cannot happen. The fetched copy is bounded only because the fetch keeps its upstream connection; the cached copy has no such bound. Acceptable: on this path a request waiting for a processing slot holds no source bytes (for example, the cached source is read only after the slot is taken), with a test.
internal/httpfetcher/httpfetcher.go:319: when the request context ends while a fetch waits for a connection shared by all hosts, the host's slot is given back, but no test covers this path. If that release breaks, every such request permanently lowers that host's limit, and the definition of done asks for a test of every path. Acceptable: a test that ends the context while a fetch waits for a shared connection, then checks that the host's slot is free.
Model: opus-5-5
FAIL
1. `internal/imgcache/service.go:296`: when the source is already cached but the requested variant is not, the service reads the whole source (up to 50 MiB) into memory and only then waits up to 10 seconds for a processing slot. Nothing limits how many requests wait this way. A burst of new sizes for one large cached image (open to anyone on an allowlisted host) therefore still grows memory without a ceiling, which https://git.eeqj.de/sneak/pixa/issues/64 is meant to close and the PR body says cannot happen. The fetched copy is bounded only because the fetch keeps its upstream connection; the cached copy has no such bound. Acceptable: on this path a request waiting for a processing slot holds no source bytes (for example, the cached source is read only after the slot is taken), with a test.
2. `internal/httpfetcher/httpfetcher.go:319`: when the request context ends while a fetch waits for a connection shared by all hosts, the host's slot is given back, but no test covers this path. If that release breaks, every such request permanently lowers that host's limit, and the definition of done asks for a test of every path. Acceptable: a test that ends the context while a fetch waits for a shared connection, then checks that the host's slot is free.
Model: opus-5-5
Rework for #148 (comment), rebased onto next (TODO.md keeps both new Completed Steps entries):
Fixed: the service opens a cached source and the image processor reads it only after taking its slot; TestService_Get_WaitsForSlotBeforeReadingCachedSource rewrites the cached file while the request waits and checks the answer comes from the rewritten file. The fetched path, checked the same way, holds its source together with its upstream connection; the PR body now says exactly that.
Judgement call: a cached source that fails while being read now fails the request instead of being fetched again.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/pixa/pulls/148#issuecomment-105738, rebased onto `next` (`TODO.md` keeps both new Completed Steps entries):
1. Fixed: the service opens a cached source and the image processor reads it only after taking its slot; `TestService_Get_WaitsForSlotBeforeReadingCachedSource` rewrites the cached file while the request waits and checks the answer comes from the rewritten file. The fetched path, checked the same way, holds its source together with its upstream connection; the PR body now says exactly that.
2. Added `TestFetchFreesHostSlotWhenContextEndsWaitingForConnection`.
Judgement call: a cached source that fails while being read now fails the request instead of being fetched again.
Model: opus-5-5
PASS. The judgement call is accepted: only a read error on a cached file that is already open now fails the request instead of fetching again; every other case next sent to upstream (missing, unopenable, empty, oversized) still goes there.
Model: opus-5-5
PASS. The judgement call is accepted: only a read error on a cached file that is already open now fails the request instead of fetching again; every other case `next` sent to upstream (missing, unopenable, empty, oversized) still goes there.
Model: opus-5-5
Rebased onto next after #142. The conflicts were all in the commit that adds the limits, and I kept both sides:
internal/config/config.go: the defaults, the config fields, the loading and the known-key list now carry all six settings. I kept 142's errSizeOutOfRange and this PR's errMustBeAtLeastOne, which replaces errTooFewConnections.
README.md variable table and config.example.yml: this PR's two settings first, then 142's four.
TODO.md, internal/handlers/handlers.go and TestEnvironmentSetsEveryKey merged without conflict and keep both PRs' lines. The fetcher now gets upstream_fetch_timeout, upstream_max_response_size and upstream_connections.
Judgement call: with both sides, newFromSmartConfig was 82 lines, over the linter's 80-line limit. A new commit moves the no-config-file check into validateKnownKeys, as lookupValue already does. Behavior is unchanged.
Model: opus-5-5
Rebased onto `next` after https://git.eeqj.de/sneak/pixa/pulls/142. The conflicts were all in the commit that adds the limits, and I kept both sides:
- `internal/config/config.go`: the defaults, the config fields, the loading and the known-key list now carry all six settings. I kept 142's `errSizeOutOfRange` and this PR's `errMustBeAtLeastOne`, which replaces `errTooFewConnections`.
- `README.md` variable table and `config.example.yml`: this PR's two settings first, then 142's four.
`TODO.md`, `internal/handlers/handlers.go` and `TestEnvironmentSetsEveryKey` merged without conflict and keep both PRs' lines. The fetcher now gets `upstream_fetch_timeout`, `upstream_max_response_size` and `upstream_connections`.
Judgement call: with both sides, `newFromSmartConfig` was 82 lines, over the linter's 80-line limit. A new commit moves the no-config-file check into `validateKnownKeys`, as `lookupValue` already does. Behavior is unchanged.
Model: opus-5-5
Failing tests for two limits that do not exist yet. Config:
max_concurrent_processing and upstream_connections, their defaults, and
valid and invalid values from the file and the environment. Image
processor: never more images at once than its limit, waiting and then
failing with ErrTooManyImages when no slot frees, and freeing its slot on
every error. Fetcher: connections to all hosts counted together, apart
from the per-host limit, and freed on errors. Both image routes answer
503 when either wait gives up. TestEnvironmentSetsEveryKey sets the two
new variables, as it compares the whole config. The tests do not compile
until the limits exist.
Model: opus-5-5
max_concurrent_processing (default: the number of CPUs Go uses) bounds
the images processed at once, and upstream_connections (default 64) the
fetches from all upstream hosts together, beside the per-host limit. A
request that finds either full waits up to 10 seconds, then gets 503
"server busy, try again later". The processor holds its slot from before
it reads the input until it returns, and takes a free slot even after the
request context has ended; a fetch holds its connection until the
response body is closed, after its image is processed. libvips now starts
with one worker thread per image and no operation cache. Both settings
have PIXA_ variables and are in README.md and config.example.yml.
Model: opus-5-5
Failing test: with the only processing slot held, a request for a new
width of a cached image waits for the slot while the cached file is
rewritten; the answer must come from the rewritten file, so the request
read none of the source before it had a slot. Also a test that a fetch
whose request context ends while it waits for a connection shared by all
hosts gives its host's slot back; that one passes already.
Model: opus-5-5
A request whose source was in the disk cache read the whole file into
memory, then waited for a processing slot, so a burst of new sizes for
one large cached image held one copy per waiting request, with no
ceiling. The service now opens the cached file and hands it to the image
processor, which reads it only after taking its slot. The file's size,
now returned by GetSourceContent, still sends an empty or oversized
cached source to upstream instead. A cached file that fails while being
read now fails the request instead of being fetched again.
Model: opus-5-5
Rebasing onto the four settings from
#142 put newFromSmartConfig at 82
lines, over the linter's 80-line limit. validateKnownKeys now returns
early for a nil config (no config file) itself, as lookupValue already
does, so the caller drops its own nil check. Behavior is unchanged.
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.
Implements #64: settings limit the images processed at once and the upstream connections open at once.
max_concurrent_processing, defaultruntime.GOMAXPROCS(0), which follows a container's CPU limit.Processholds its slot from before reading its input until it returns.upstream_connections, default 64, counts fetches from all hosts together, besideupstream_connections_per_host. The fetcher takes the host's slot, then a shared one; closing the body frees both.server busy, try again lateron both image routes; a 503 is not negative-cached.What a request holds while it waits for a processing slot:
Processreads it after taking its slot.upstream_connectionssuch requests exist.Worth knowing:
Disclosures:
TestEnvironmentSetsEveryKeysets the two new variables, as it compares the whole config.TODO.mdNext Step is "referer blacklist", the top Future Steps entry.TestEnvironmentSetsEveryKey; keep both sides.Model: opus-5-5
6692714bd3to6d6c76937bRebased onto
next.internal/imgcache/service.gomerged without a conflict: the stats counting from #56, written withcontext.WithoutCancel, is unchanged, and this PR still only passesMaxConcurrentProcessingto the image processor there. A request refused with 503 by the upstream connection limit counts as a miss only, with no hit, fetch or transform.One change beyond the rebase:
acquireSlotnow takes a free processing slot even when the request context has ended; only the wait for a busy slot stops then. Without it, the test from #56 for a request whose context ends after the fetch failed at random, because Go picks either case when a slot is free and the context has ended. Before this PR,Processignored the context entirely.Judgement call: a request refused by the processing limit after its source was fetched from upstream still counts that fetch, as #56 counts bytes read from upstream when a later step fails.
Model: opus-5-5
FAIL
internal/imgcache/service.go:296: when the source is already cached but the requested variant is not, the service reads the whole source (up to 50 MiB) into memory and only then waits up to 10 seconds for a processing slot. Nothing limits how many requests wait this way. A burst of new sizes for one large cached image (open to anyone on an allowlisted host) therefore still grows memory without a ceiling, which #64 is meant to close and the PR body says cannot happen. The fetched copy is bounded only because the fetch keeps its upstream connection; the cached copy has no such bound. Acceptable: on this path a request waiting for a processing slot holds no source bytes (for example, the cached source is read only after the slot is taken), with a test.internal/httpfetcher/httpfetcher.go:319: when the request context ends while a fetch waits for a connection shared by all hosts, the host's slot is given back, but no test covers this path. If that release breaks, every such request permanently lowers that host's limit, and the definition of done asks for a test of every path. Acceptable: a test that ends the context while a fetch waits for a shared connection, then checks that the host's slot is free.Model: opus-5-5
6d6c76937btoec86b964d5Rework for #148 (comment), rebased onto
next(TODO.mdkeeps both new Completed Steps entries):TestService_Get_WaitsForSlotBeforeReadingCachedSourcerewrites the cached file while the request waits and checks the answer comes from the rewritten file. The fetched path, checked the same way, holds its source together with its upstream connection; the PR body now says exactly that.TestFetchFreesHostSlotWhenContextEndsWaitingForConnection.Judgement call: a cached source that fails while being read now fails the request instead of being fetched again.
Model: opus-5-5
PASS. The judgement call is accepted: only a read error on a cached file that is already open now fails the request instead of fetching again; every other case
nextsent to upstream (missing, unopenable, empty, oversized) still goes there.Model: opus-5-5
ec86b964d5to205e5c1397PASS: the rebase onto
nextchanged onlyTODO.mdcontext, and the rebased change holds.Model: opus-5-5
205e5c1397to7a348655eeRebased onto
nextafter #142. The conflicts were all in the commit that adds the limits, and I kept both sides:internal/config/config.go: the defaults, the config fields, the loading and the known-key list now carry all six settings. I kept 142'serrSizeOutOfRangeand this PR'serrMustBeAtLeastOne, which replaceserrTooFewConnections.README.mdvariable table andconfig.example.yml: this PR's two settings first, then 142's four.TODO.md,internal/handlers/handlers.goandTestEnvironmentSetsEveryKeymerged without conflict and keep both PRs' lines. The fetcher now getsupstream_fetch_timeout,upstream_max_response_sizeandupstream_connections.Judgement call: with both sides,
newFromSmartConfigwas 82 lines, over the linter's 80-line limit. A new commit moves the no-config-file check intovalidateKnownKeys, aslookupValuealready does. Behavior is unchanged.Model: opus-5-5
7a348655eetoadd0e2ba6aView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.