diff --git a/README.md b/README.md index 9dc720d..1bbd2ee 100644 --- a/README.md +++ b/README.md @@ -314,9 +314,23 @@ cache at once, they share one upstream fetch (or one read of the cached source) and one transcode: the first request does the work, and the others wait for its image or its error, holding no upstream connection or processing slot of their own. A waiting request stops waiting when its own client goes away. The work -goes on for the others even if the first request's client goes away, until that -request's `downstream_timeout` ends. The shared fetch sends the first request's -ID upstream, and the lines logged for the fetch and the transcode carry that ID. +goes on for the others even if the first request's client goes away. The shared +fetch sends the first request's ID upstream, and the lines logged for the fetch +and the transcode carry that ID. + +A transcode whose request's `downstream_timeout` has ended (for a shared one, +the first request's) stops when it gets its processing slot or before it begins +encoding, and gives the slot back. libvips cannot stop an encode once it has +begun, so an encode keeps its slot until it finishes, which can be after +`downstream_timeout`. The limit of 8192 on each side of the source and of the +output bounds how long that takes. With one libvips thread, as pixad runs it, +encoding 8192x8192 random pixels takes up to about 1 second as JPEG, 15 as PNG, +16 as GIF, 63 as WebP, 110 as AVIF and 96 as JPEG XL, the slowest of 8 or 16 +bits per sample, with or without an alpha channel. WebP, AVIF and JPEG XL are +that slow only with an alpha channel: 8-bit RGB takes about 44, 53 and 28 +seconds. Decoding such a source adds up to about 11 seconds. A WebP, AVIF or +JPEG XL encode can therefore hold its slot past the default `downstream_timeout` +of 60 seconds. The login form (`POST /`) is limited to 5 attempts per minute per client address, counting an IPv6 client by its /64; an attempt over the limit is diff --git a/TODO.md b/TODO.md index 8d09a38..af7ab63 100644 --- a/TODO.md +++ b/TODO.md @@ -70,6 +70,21 @@ P2: security: per-origin rate limiting and the query now have each `%` written as `%25` and each `:` as `%3A`; a path and query holding neither are signed and keyed as before. Signature Specification in `README.md` says so. +- 2026-10-15 a transcode stops once its request's `downstream_timeout` has + ended, unless it has begun encoding (closes #243): the image processor looked + at its context only while it waited for a processing slot, so an image was + decoded and encoded, holding the slot, however long after its request's + deadline that took. `Process` now returns the context's error, and gives the + slot back, when the context has ended by the time it has the slot or before it + encodes. libvips cannot stop an encode once it has begun, so that encode keeps + its slot until it finishes; the limit of 8192 on each side of the source and + of the output bounds how long that takes. With one libvips thread, 8192x8192 + random pixels take up to about 1 second to encode as JPEG, 15 as PNG, 16 as + GIF, 63 as WebP, 110 as AVIF and 96 as JPEG XL, the last three with an alpha + channel, plus up to about 11 seconds to decode the source; `README.md` gives + these times and no longer says the work stops when `downstream_timeout` ends. + The settings of the WebP, AVIF and JPEG XL encoders, which can hold a slot + past the default `downstream_timeout` of 60 seconds, are unchanged. - 2026-10-15 the example config allowlists no host (closes #249): it allowlisted `github.com` and `user-images.githubusercontent.com`, where anyone can put an image, so a deployment set up as Getting Started says served those images to @@ -533,11 +548,11 @@ P2: security: per-origin rate limiting cancellation but keeps its deadline, and the others wait for its image or error holding no upstream connection or processing slot, and stop waiting when their own context ends; the request doing the processing waits for it even - then, up to its deadline; a request whose context has already ended starts - nothing; each request counts one miss, and the processing counts its fetch and - transcode once; a panic while processing is reported to Sentry when - `sentry_dsn` is set and becomes an error for every waiting request instead of - stopping pixad; documented in `README.md`. + then, until it ends, which can be after its deadline; a request whose context + has already ended starts nothing; each request counts one miss, and the + processing counts its fetch and transcode once; a panic while processing is + reported to Sentry when `sentry_dsn` is set and becomes an error for every + waiting request instead of stopping pixad; documented in `README.md`. - 2026-09-29 only the image routes send CORS headers (closes #98): the CORS middleware, with the `access_control_allow_origin` origin, moved from the router root onto a `/v1` subrouter holding `/v1/image/` and `/v1/e/`, where it diff --git a/internal/imageprocessor/imageprocessor.go b/internal/imageprocessor/imageprocessor.go index 92cee8a..4a18aac 100644 --- a/internal/imageprocessor/imageprocessor.go +++ b/internal/imageprocessor/imageprocessor.go @@ -208,7 +208,9 @@ func New(params Params) *ImageProcessor { // Process transforms an image according to the request. When // MaxConcurrentProcessing images are already being processed, it waits up // to ProcessingWaitTimeout for one to finish, then fails with -// ErrTooManyImages. +// ErrTooManyImages. When ctx has ended once Process has a slot, or before it +// encodes, it returns ctx's error and gives the slot back. libvips cannot stop +// an encode once it has begun, so an encode keeps its slot until it finishes. func (p *ImageProcessor) Process( ctx context.Context, input io.Reader, @@ -290,7 +292,7 @@ func (p *ImageProcessor) Process( } // Encode to target format - output, err := p.encode(img, outputFormat, req.Quality) + output, err := p.encode(ctx, img, outputFormat, req.Quality) if err != nil { return nil, fmt.Errorf("failed to encode: %w", err) } @@ -417,25 +419,28 @@ func (p *ImageProcessor) WaitForProcessing(ctx context.Context) int { // acquireSlot takes a slot in processingSemaphore, waiting at most // processingWaitTimeout for one to free up, and returns the func that gives -// it back. A free slot is taken even when ctx has ended; only the wait for -// one stops when ctx ends, as the rest of Process does not check ctx. +// it back. When ctx has ended by the time it has a slot, it gives the slot +// back and returns ctx's error. func (p *ImageProcessor) acquireSlot(ctx context.Context) (func(), error) { - release := func() { <-p.processingSemaphore } - select { case p.processingSemaphore <- struct{}{}: - return release, nil - default: - } - - select { - case p.processingSemaphore <- struct{}{}: - return release, nil case <-time.After(p.processingWaitTimeout): return nil, ErrTooManyImages case <-ctx.Done(): return nil, ctx.Err() } + + release := func() { <-p.processingSemaphore } + + // select takes the slot even when ctx has ended at the same moment + err := ctx.Err() + if err != nil { + release() + + return nil, err + } + + return release, nil } // detectFormat returns the format string for a govips image type. @@ -527,10 +532,17 @@ func (p *ImageProcessor) resize( const defaultQuality = 85 -// encode encodes an image to the specified format. +// encode encodes an image to the specified format. libvips cannot stop an +// encode once it has begun, so when ctx has ended encode returns ctx's error +// without beginning one. func (p *ImageProcessor) encode( - img *vips.ImageRef, format Format, quality int, + ctx context.Context, img *vips.ImageRef, format Format, quality int, ) ([]byte, error) { + err := ctx.Err() + if err != nil { + return nil, err + } + if quality <= 0 { quality = defaultQuality } @@ -540,7 +552,7 @@ func (p *ImageProcessor) encode( // libvips' built-in profile; govips' own sRGB path variable is set on // first use but read without a lock, so concurrent requests race on it. if img.HasICCProfile() { - err := img.TransformICCProfileWithFallback("srgb", "srgb") + err = img.TransformICCProfileWithFallback("srgb", "srgb") if err != nil { return nil, fmt.Errorf("failed to convert to sRGB: %w", err) }