Service.Get (internal/imgcache/service.go:103-158) has no singleflight or equivalent. K simultaneous requests for the same uncached variant each miss the cache, each fetch the source from the upstream, and each run a full libvips transcode, before K identical results race to write the same cache entry.
This is the classic thundering herd, and it is the worst case for exactly the traffic pattern an image CDN sees: a popular image going cold, or a cache-cleared restart under live traffic. It multiplies both the upstream load we impose on origins and our own CPU/memory use, and it directly contradicts the 1-5k req/s target in README.md:49.
It also interacts badly with the concurrency cap filed separately: without coalescing, the cap gets consumed by duplicate work.
Definition of done
Concurrent requests for the same variant cache key collapse to a single upstream fetch and a single transcode; the rest wait and receive the same result. Use golang.org/x/sync/singleflight rather than hand-rolling (per the stdlib/well-maintained-library preference — golang.org/x/sync is already the closest thing to stdlib here; confirm it against ~/.claude/GO_PACKAGE_DEFAULTS.md before adding, and add the decision there if absent).
Coalescing keys on the full variant cache key (including quality and fit), not just the source URL — two different requested variants of the same source must not be collapsed into one another.
An error in the leader does not poison waiters indefinitely; verify the negative-cache interaction (internal/imgcache/cache.go:214) still behaves.
No deadlock or goroutine leak when a waiter's context is cancelled — the leader must still complete or be cancelled cleanly.
Failing test first: N concurrent Get calls for the same uncached key result in exactly one fetch against a counting mock fetcher (internal/httpfetcher.MockFetcher already exists), and all N callers get correct bytes.
make check green (suite runs with -race as of #55).
Related
Best implemented alongside the global concurrency cap issue; both touch Service.Get and both target the same blowup.
Verified against `main` at `61f42e6`.
`Service.Get` (`internal/imgcache/service.go:103-158`) has no singleflight or equivalent. K simultaneous requests for the same uncached variant each miss the cache, each fetch the source from the upstream, and each run a full libvips transcode, before K identical results race to write the same cache entry.
This is the classic thundering herd, and it is the worst case for exactly the traffic pattern an image CDN sees: a popular image going cold, or a cache-cleared restart under live traffic. It multiplies both the upstream load we impose on origins and our own CPU/memory use, and it directly contradicts the 1-5k req/s target in `README.md:49`.
It also interacts badly with the concurrency cap filed separately: without coalescing, the cap gets consumed by duplicate work.
## Definition of done
1. Concurrent requests for the same variant cache key collapse to a single upstream fetch and a single transcode; the rest wait and receive the same result. Use `golang.org/x/sync/singleflight` rather than hand-rolling (per the stdlib/well-maintained-library preference — `golang.org/x/sync` is already the closest thing to stdlib here; confirm it against `~/.claude/GO_PACKAGE_DEFAULTS.md` before adding, and add the decision there if absent).
2. Coalescing keys on the full variant cache key (including quality and fit), not just the source URL — two different requested variants of the same source must not be collapsed into one another.
3. An error in the leader does not poison waiters indefinitely; verify the negative-cache interaction (`internal/imgcache/cache.go:214`) still behaves.
4. No deadlock or goroutine leak when a waiter's context is cancelled — the leader must still complete or be cancelled cleanly.
5. Failing test first: N concurrent `Get` calls for the same uncached key result in exactly one fetch against a counting mock fetcher (`internal/httpfetcher.MockFetcher` already exists), and all N callers get correct bytes.
6. `make check` green (suite runs with `-race` as of #55).
## Related
- Best implemented alongside the global concurrency cap issue; both touch `Service.Get` and both target the same blowup.
clawbot
added this to the 1.0.0 milestone 2026-08-09 03:44:03 +02:00
Plan, checked against current next (Service.Get is now at internal/imgcache/service.go:118-179):
A cache miss goes through a singleflight.Group (golang.org/x/sync/singleflight) keyed on CacheKey(req), which covers host, path, query, size, format, quality and fit. The first request's function runs the existing path unchanged (cached source or upstream fetch, transcode, store), so it takes the upstream connection and processing slot exactly as it does now. Requests for the same key that arrive meanwhile wait for its result: they fetch nothing, read no source and take no slot. Each gets its own reader over the one set of processed bytes.
Context: the shared function runs with context.WithoutCancel of the first request's context, so the others are still served if that client goes away; it stays bounded by upstream_fetch_timeout and the two 10-second waits. A request waiting for another request's work returns as soon as its own context ends (DoChan with a select); the work goes on. The request whose own function is running waits for it even when its context ends, as every request does on next: the test from #56 for a context that ends after the fetch expects that request to be served, and the repo's rules keep an existing test unchanged without the owner's approval. A request whose context has already ended starts no work.
Counting: every request counts one miss, the waiting ones included. The shared function counts the upstream fetch and the transcode, once each. IncrementStats keeps its signature (existing tests call it with bytes); the fetch count moves into a method of its own that IncrementStats also uses.
Errors: every waiting request gets the first request's error. The key is dropped when the function returns, so the next request starts afresh. The negative cache entry is written once, by the shared function, as now.
Panics: singleflight raises a panic from the shared function again in a goroutine of its own, where the router's recoverer cannot catch it, so one bad image would stop pixad. The function turns a panic into an error for the requests waiting on it and logs the stack.
Tests first, in their own commit: N concurrent requests for one uncached variant make one fetch and one transcode, all get the same correct image, and the counters show N misses; variants differing only in quality or fit are not merged; a waiting request whose context ends returns while the fetch is still held; when the first request's client goes away the others are still served; a shared upstream error reaches everyone, the negative cache answers the next request, and a failure that is not negatively cached is fetched again; a panic becomes an error.
golang.org/x/sync (already in go.sum at v0.19.0) moves to a direct dependency; the decision goes into the owner's Go package defaults. One sentence in README.md, and the TODO.md entry.
Model: opus-5-5
Plan, checked against current `next` (`Service.Get` is now at `internal/imgcache/service.go:118-179`):
- A cache miss goes through a `singleflight.Group` (`golang.org/x/sync/singleflight`) keyed on `CacheKey(req)`, which covers host, path, query, size, format, quality and fit. The first request's function runs the existing path unchanged (cached source or upstream fetch, transcode, store), so it takes the upstream connection and processing slot exactly as it does now. Requests for the same key that arrive meanwhile wait for its result: they fetch nothing, read no source and take no slot. Each gets its own reader over the one set of processed bytes.
- Context: the shared function runs with `context.WithoutCancel` of the first request's context, so the others are still served if that client goes away; it stays bounded by `upstream_fetch_timeout` and the two 10-second waits. A request waiting for another request's work returns as soon as its own context ends (`DoChan` with a `select`); the work goes on. The request whose own function is running waits for it even when its context ends, as every request does on `next`: the test from https://git.eeqj.de/sneak/pixa/issues/56 for a context that ends after the fetch expects that request to be served, and the repo's rules keep an existing test unchanged without the owner's approval. A request whose context has already ended starts no work.
- Counting: every request counts one miss, the waiting ones included. The shared function counts the upstream fetch and the transcode, once each. `IncrementStats` keeps its signature (existing tests call it with bytes); the fetch count moves into a method of its own that `IncrementStats` also uses.
- Errors: every waiting request gets the first request's error. The key is dropped when the function returns, so the next request starts afresh. The negative cache entry is written once, by the shared function, as now.
- Panics: singleflight raises a panic from the shared function again in a goroutine of its own, where the router's recoverer cannot catch it, so one bad image would stop pixad. The function turns a panic into an error for the requests waiting on it and logs the stack.
- Tests first, in their own commit: N concurrent requests for one uncached variant make one fetch and one transcode, all get the same correct image, and the counters show N misses; variants differing only in quality or fit are not merged; a waiting request whose context ends returns while the fetch is still held; when the first request's client goes away the others are still served; a shared upstream error reaches everyone, the negative cache answers the next request, and a failure that is not negatively cached is fetched again; a panic becomes an error.
- `golang.org/x/sync` (already in `go.sum` at v0.19.0) moves to a direct dependency; the decision goes into the owner's Go package defaults. One sentence in `README.md`, and the `TODO.md` entry.
Model: opus-5-5
Built in #160 as planned above. The one point left for the owner is in its body: the request running the shared work still waits for it when its own context ends, because letting it leave would change an existing test from #56.
Model: opus-5-5
Built in https://git.eeqj.de/sneak/pixa/pulls/160 as planned above. The one point left for the owner is in its body: the request running the shared work still waits for it when its own context ends, because letting it leave would change an existing test from https://git.eeqj.de/sneak/pixa/issues/56.
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.Service.Get(internal/imgcache/service.go:103-158) has no singleflight or equivalent. K simultaneous requests for the same uncached variant each miss the cache, each fetch the source from the upstream, and each run a full libvips transcode, before K identical results race to write the same cache entry.This is the classic thundering herd, and it is the worst case for exactly the traffic pattern an image CDN sees: a popular image going cold, or a cache-cleared restart under live traffic. It multiplies both the upstream load we impose on origins and our own CPU/memory use, and it directly contradicts the 1-5k req/s target in
README.md:49.It also interacts badly with the concurrency cap filed separately: without coalescing, the cap gets consumed by duplicate work.
Definition of done
golang.org/x/sync/singleflightrather than hand-rolling (per the stdlib/well-maintained-library preference —golang.org/x/syncis already the closest thing to stdlib here; confirm it against~/.claude/GO_PACKAGE_DEFAULTS.mdbefore adding, and add the decision there if absent).internal/imgcache/cache.go:214) still behaves.Getcalls for the same uncached key result in exactly one fetch against a counting mock fetcher (internal/httpfetcher.MockFetcheralready exists), and all N callers get correct bytes.make checkgreen (suite runs with-raceas of #55).Related
Service.Getand both target the same blowup.Plan, checked against current
next(Service.Getis now atinternal/imgcache/service.go:118-179):singleflight.Group(golang.org/x/sync/singleflight) keyed onCacheKey(req), which covers host, path, query, size, format, quality and fit. The first request's function runs the existing path unchanged (cached source or upstream fetch, transcode, store), so it takes the upstream connection and processing slot exactly as it does now. Requests for the same key that arrive meanwhile wait for its result: they fetch nothing, read no source and take no slot. Each gets its own reader over the one set of processed bytes.context.WithoutCancelof the first request's context, so the others are still served if that client goes away; it stays bounded byupstream_fetch_timeoutand the two 10-second waits. A request waiting for another request's work returns as soon as its own context ends (DoChanwith aselect); the work goes on. The request whose own function is running waits for it even when its context ends, as every request does onnext: the test from #56 for a context that ends after the fetch expects that request to be served, and the repo's rules keep an existing test unchanged without the owner's approval. A request whose context has already ended starts no work.IncrementStatskeeps its signature (existing tests call it with bytes); the fetch count moves into a method of its own thatIncrementStatsalso uses.golang.org/x/sync(already ingo.sumat v0.19.0) moves to a direct dependency; the decision goes into the owner's Go package defaults. One sentence inREADME.md, and theTODO.mdentry.Model: opus-5-5
Built in #160 as planned above. The one point left for the owner is in its body: the request running the shared work still waits for it when its own context ends, because letting it leave would change an existing test from #56.
Model: opus-5-5
clawbot referenced this issue2026-10-04 02:10:22 +02:00