No request coalescing: K concurrent misses for the same variant cause K upstream fetches and K transcodes #65

New Issue

2026-08-09T03:44:03+02:00

clawbot commented

2026-08-09 03:44:03 +02:00

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

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).

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

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#65