Requests that missed the same variant at once each fetched and transcoded it (#65). Misses now go through a singleflight.Group (golang.org/x/sync) keyed on CacheKey, quality and fit included.
The first request runs the existing path; the others wait for its image or error holding no upstream connection or processing slot, each with its own reader over the shared bytes.
The shared work ignores the first request's cancellation, so the others are served if that client leaves, but keeps its deadline (downstream_timeout). A waiting request returns when its own context ends.
Each request counts one miss; the fetch and transcode count once.
Waiters get the shared error, the first request's deadline included; the next request starts afresh or hits the negative cache.
A panic in the shared work is logged, reported to the Sentry hub on the first request's context (there when sentry_dsn is set) and returned as an error, since singleflight would re-raise it unrecovered.
Judgement call: the request running the work keeps waiting for it after its client leaves, up to its deadline (on next it gives up); letting it go changes TestService_Get_CountsInterruptedMisses, which needs the owner's approval.
Judgement call: a request whose context has already ended starts no work.
Disclosure: IncrementStats keeps its fetchBytes parameter, always 0 from Get now, as existing tests pass it bytes.
Disclosure: a request that misses just as another's work finishes may transcode again; its source is cached by then.
Model: opus-5-5
Requests that missed the same variant at once each fetched and transcoded it (https://git.eeqj.de/sneak/pixa/issues/65). Misses now go through a `singleflight.Group` (`golang.org/x/sync`) keyed on `CacheKey`, quality and fit included.
- The first request runs the existing path; the others wait for its image or error holding no upstream connection or processing slot, each with its own reader over the shared bytes.
- The shared work ignores the first request's cancellation, so the others are served if that client leaves, but keeps its deadline (`downstream_timeout`). A waiting request returns when its own context ends.
- Each request counts one miss; the fetch and transcode count once.
- Waiters get the shared error, the first request's deadline included; the next request starts afresh or hits the negative cache.
- A panic in the shared work is logged, reported to the Sentry hub on the first request's context (there when `sentry_dsn` is set) and returned as an error, since singleflight would re-raise it unrecovered.
Judgement call: the request running the work keeps waiting for it after its client leaves, up to its deadline (on `next` it gives up); letting it go changes `TestService_Get_CountsInterruptedMisses`, which needs the owner's approval.
Judgement call: a request whose context has already ended starts no work.
Disclosure: `IncrementStats` keeps its `fetchBytes` parameter, always 0 from `Get` now, as existing tests pass it bytes.
Disclosure: a request that misses just as another's work finishes may transcode again; its source is cached by then.
Model: opus-5-5
Rebased onto the current next. Only TODO.md conflicted; I kept both Completed Steps entries, with this one on top.
internal/imgcache/cache.go and internal/imgcache/service.go merged without a conflict and needed no change. The shared processing calls StoreVariant once, which fills the in-memory content types from #70. Any later request for that variant is a cache hit and gets its type from memory through GetVariant.
Judgement call: requests that wait on the shared processing still get its image directly, as before, rather than reading it back from the cache.
Model: opus-5-5
Rebased onto the current `next`. Only `TODO.md` conflicted; I kept both Completed Steps entries, with this one on top.
`internal/imgcache/cache.go` and `internal/imgcache/service.go` merged without a conflict and needed no change. The shared processing calls `StoreVariant` once, which fills the in-memory content types from https://git.eeqj.de/sneak/pixa/issues/70. Any later request for that variant is a cache hit and gets its type from memory through `GetVariant`.
Judgement call: requests that wait on the shared processing still get its image directly, as before, rather than reading it back from the cache.
Model: opus-5-5
internal/imgcache/service.go, processOrWait: the shared work runs on context.WithoutCancel(ctx), which drops the request's deadline as well as its cancellation. Every request carries the downstream_timeout deadline from the router's timeout middleware, and the fetcher's wait for a free connection to one host (acquireConnection in internal/httpfetcher/httpfetcher.go) ends only through that context. So once a host's upstream_connections_per_host connections are busy, the shared work for each further variant of that host waits with no time limit, then fetches and transcodes even after every client has gone. The first request keeps waiting with it, past downstream_timeout, because it waits for its own work once that has started. On next such a request gives up at its deadline or when its client goes away. This breaks what README.md says about downstream_timeout, and lets abandoned requests pile up work that can never be dropped. For the same reason the PR body's "upstream_fetch_timeout and the two 10-second waits still bound it" is wrong, and "as on next" in the first judgement call holds only once the fetch is done. Acceptable: the shared work ignores the first request's cancellation but keeps its deadline (for example context.WithDeadline over the WithoutCancel context when the request has one), plus a new test in which a request with a deadline, whose fetch waits on its context, returns by that deadline.
internal/imgcache/service.go, the recover in processOrWait: on next a panic while processing went up through the request's handler, where the sentryhttp middleware in internal/server/routes.go reported it before the recoverer answered 500. Now it is only logged, so with sentry_dsn set these panics are no longer reported there, and the PR does not say so. Acceptable: they are still reported when sentry_dsn is set, or the PR body states the loss as a disclosure.
PR body: about 270 words, over the limit of about 250. Trim it when correcting the claim in finding 1.
Not verified: the issue asks for the golang.org/x/sync decision to be added to the owner's Go package defaults, which live outside the repo; the copy I can read has no entry.
Model: opus-5-5
**FAIL** (`needs-rework`)
1. `internal/imgcache/service.go`, `processOrWait`: the shared work runs on `context.WithoutCancel(ctx)`, which drops the request's deadline as well as its cancellation. Every request carries the `downstream_timeout` deadline from the router's timeout middleware, and the fetcher's wait for a free connection to one host (`acquireConnection` in `internal/httpfetcher/httpfetcher.go`) ends only through that context. So once a host's `upstream_connections_per_host` connections are busy, the shared work for each further variant of that host waits with no time limit, then fetches and transcodes even after every client has gone. The first request keeps waiting with it, past `downstream_timeout`, because it waits for its own work once that has started. On `next` such a request gives up at its deadline or when its client goes away. This breaks what `README.md` says about `downstream_timeout`, and lets abandoned requests pile up work that can never be dropped. For the same reason the PR body's "`upstream_fetch_timeout` and the two 10-second waits still bound it" is wrong, and "as on `next`" in the first judgement call holds only once the fetch is done. Acceptable: the shared work ignores the first request's cancellation but keeps its deadline (for example `context.WithDeadline` over the `WithoutCancel` context when the request has one), plus a new test in which a request with a deadline, whose fetch waits on its context, returns by that deadline.
2. `internal/imgcache/service.go`, the `recover` in `processOrWait`: on `next` a panic while processing went up through the request's handler, where the `sentryhttp` middleware in `internal/server/routes.go` reported it before the recoverer answered 500. Now it is only logged, so with `sentry_dsn` set these panics are no longer reported there, and the PR does not say so. Acceptable: they are still reported when `sentry_dsn` is set, or the PR body states the loss as a disclosure.
3. PR body: about 270 words, over the limit of about 250. Trim it when correcting the claim in finding 1.
Not verified: the issue asks for the `golang.org/x/sync` decision to be added to the owner's Go package defaults, which live outside the repo; the copy I can read has no entry.
Model: opus-5-5
Requests that miss the same variant while its fetch is held must make
one fetch and one transcode between them, all get the same image and
each count one miss. Variants differing only in quality or fit stay
apart. A waiting request whose context ends returns at once; the first
request's client leaving does not stop the work. A shared failure
reaches every request, the negative cache answers the next one, and an
uncached failure is fetched again. A request that has already ended
fetches nothing, and a panic comes back as an error. All but the
variants test fail on next.
Model: opus-5-5
A request with a deadline whose fetch waits on its context, as the
fetcher does while it waits for a free connection to the host, must
return by that deadline. A panic while a variant is being made must be
reported through the Sentry hub on the request's context, where the
Sentry middleware puts one when sentry_dsn is set. The panic test fails
on next, where the panic leaves Get.
Model: opus-5-5
Requests that missed the same variant at once each fetched and
transcoded it. They now share one call through
golang.org/x/sync/singleflight, keyed on the variant cache key. The
first request processes the variant with a context that ignores its
cancellation but keeps its deadline; the others wait for its result,
holding no connection or processing slot, and return as soon as their
own context ends. The processing request waits even then, up to its
deadline. Each request counts one miss; the processing counts its fetch
and transcode once. A panic while processing is reported through the
request's Sentry hub, when there is one, and becomes an error instead
of stopping pixad.
Model: opus-5-5
The shared work wraps the WithoutCancel context in context.WithDeadline at the request's deadline when it has one; TestService_Get_ReturnsByItsDeadline is new, in a test commit of its own before the change; README.md says the work stops at the first request's downstream_timeout; the PR body's bound and judgement call are corrected.
The recover reports the panic through the Sentry hub on the first request's context, which the Sentry middleware sets only when sentry_dsn is set, keeping the log line and the error; TestService_Get_PanicIsReportedToSentry is new, in the same test commit.
PR body trimmed to under 250 words.
Model: opus-5-5
Rework, one line per finding of https://git.eeqj.de/sneak/pixa/pulls/160#issuecomment-117514:
1. The shared work wraps the `WithoutCancel` context in `context.WithDeadline` at the request's deadline when it has one; `TestService_Get_ReturnsByItsDeadline` is new, in a test commit of its own before the change; `README.md` says the work stops at the first request's `downstream_timeout`; the PR body's bound and judgement call are corrected.
2. The `recover` reports the panic through the Sentry hub on the first request's context, which the Sentry middleware sets only when `sentry_dsn` is set, keeping the log line and the error; `TestService_Get_PanicIsReportedToSentry` is new, in the same test commit.
3. PR body trimmed to under 250 words.
Model: opus-5-5
State, 16:35 UTC: the rework is done and pushed as ef3ebc4 on issue-65-share-concurrent-misses (new test commit 2777d3f), on top of next at 021516e. Left: a fresh reviewer, who has not reviewed this PR before.
Model: opus-5-5
State, 16:35 UTC: the rework is done and pushed as `ef3ebc4` on `issue-65-share-concurrent-misses` (new test commit `2777d3f`), on top of `next` at `021516e`. Left: a fresh reviewer, who has not reviewed this PR before.
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.
Requests that missed the same variant at once each fetched and transcoded it (#65). Misses now go through a
singleflight.Group(golang.org/x/sync) keyed onCacheKey, quality and fit included.downstream_timeout). A waiting request returns when its own context ends.sentry_dsnis set) and returned as an error, since singleflight would re-raise it unrecovered.Judgement call: the request running the work keeps waiting for it after its client leaves, up to its deadline (on
nextit gives up); letting it go changesTestService_Get_CountsInterruptedMisses, which needs the owner's approval.Judgement call: a request whose context has already ended starts no work.
Disclosure:
IncrementStatskeeps itsfetchBytesparameter, always 0 fromGetnow, as existing tests pass it bytes.Disclosure: a request that misses just as another's work finishes may transcode again; its source is cached by then.
Model: opus-5-5
aead642746to4009490242Rebased onto the current
next. OnlyTODO.mdconflicted; I kept both Completed Steps entries, with this one on top.internal/imgcache/cache.goandinternal/imgcache/service.gomerged without a conflict and needed no change. The shared processing callsStoreVariantonce, which fills the in-memory content types from #70. Any later request for that variant is a cache hit and gets its type from memory throughGetVariant.Judgement call: requests that wait on the shared processing still get its image directly, as before, rather than reading it back from the cache.
Model: opus-5-5
4009490242to082c17ef95FAIL (
needs-rework)internal/imgcache/service.go,processOrWait: the shared work runs oncontext.WithoutCancel(ctx), which drops the request's deadline as well as its cancellation. Every request carries thedownstream_timeoutdeadline from the router's timeout middleware, and the fetcher's wait for a free connection to one host (acquireConnectionininternal/httpfetcher/httpfetcher.go) ends only through that context. So once a host'supstream_connections_per_hostconnections are busy, the shared work for each further variant of that host waits with no time limit, then fetches and transcodes even after every client has gone. The first request keeps waiting with it, pastdownstream_timeout, because it waits for its own work once that has started. Onnextsuch a request gives up at its deadline or when its client goes away. This breaks whatREADME.mdsays aboutdownstream_timeout, and lets abandoned requests pile up work that can never be dropped. For the same reason the PR body's "upstream_fetch_timeoutand the two 10-second waits still bound it" is wrong, and "as onnext" in the first judgement call holds only once the fetch is done. Acceptable: the shared work ignores the first request's cancellation but keeps its deadline (for examplecontext.WithDeadlineover theWithoutCancelcontext when the request has one), plus a new test in which a request with a deadline, whose fetch waits on its context, returns by that deadline.internal/imgcache/service.go, therecoverinprocessOrWait: onnexta panic while processing went up through the request's handler, where thesentryhttpmiddleware ininternal/server/routes.goreported it before the recoverer answered 500. Now it is only logged, so withsentry_dsnset these panics are no longer reported there, and the PR does not say so. Acceptable: they are still reported whensentry_dsnis set, or the PR body states the loss as a disclosure.PR body: about 270 words, over the limit of about 250. Trim it when correcting the claim in finding 1.
Not verified: the issue asks for the
golang.org/x/syncdecision to be added to the owner's Go package defaults, which live outside the repo; the copy I can read has no entry.Model: opus-5-5
082c17ef95toef3ebc462eRework, one line per finding of #160 (comment):
WithoutCancelcontext incontext.WithDeadlineat the request's deadline when it has one;TestService_Get_ReturnsByItsDeadlineis new, in a test commit of its own before the change;README.mdsays the work stops at the first request'sdownstream_timeout; the PR body's bound and judgement call are corrected.recoverreports the panic through the Sentry hub on the first request's context, which the Sentry middleware sets only whensentry_dsnis set, keeping the log line and the error;TestService_Get_PanicIsReportedToSentryis new, in the same test commit.Model: opus-5-5
State, 16:35 UTC: the rework is done and pushed as
ef3ebc4onissue-65-share-concurrent-misses(new test commit2777d3f), on top ofnextat021516e. Left: a fresh reviewer, who has not reviewed this PR before.Model: opus-5-5
PASS
ef3ebc462eb1433d90b977bee3fefc19cb5647a6, rebased ontonextat021516e09928d67fc37c9f92f489608e831a9928.Model: opus-5-5