CHANGELOG.md

@@ -9,6 +9,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 
 *Nothing yet.*
 
+## [1.3.2] - September 13, 2023
+
+### Fixed
+
+* Fixed thumbnail generation causing `thumbnails_index` errors in some circumstances.
+
 ## [1.3.1] - September 8, 2023
 
 ### Fixed
@@ -447,7 +453,8 @@ a large database (more than about 100k uploaded files), run the following steps
 * Various other features that would be expected like maximum/minimum size controls, rate limiting, etc. Check out the
   sample config for a better idea of what else is possible.
 
-[unreleased]: https://github.com/turt2live/matrix-media-repo/compare/v1.3.1...HEAD
+[unreleased]: https://github.com/turt2live/matrix-media-repo/compare/v1.3.2...HEAD
+[1.3.2]: https://github.com/turt2live/matrix-media-repo/compare/v1.3.1...v1.3.2
 [1.3.1]: https://github.com/turt2live/matrix-media-repo/compare/v1.3.0...v1.3.1
 [1.3.0]: https://github.com/turt2live/matrix-media-repo/compare/v1.2.13...v1.3.0
 [1.2.13]: https://github.com/turt2live/matrix-media-repo/compare/v1.2.12...v1.2.13

pipelines/_steps/thumbnails/generate.go

@@ -5,6 +5,7 @@ import (
 	"io"
 	"strconv"
 
+	"github.com/getsentry/sentry-go"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/turt2live/matrix-media-repo/common"
 	"github.com/turt2live/matrix-media-repo/common/rcontext"
@@ -70,6 +71,42 @@ func Generate(ctx rcontext.RequestContext, mediaRecord *database.DbMedia, width
 
 	// At this point, res.i is our thumbnail
 
+	// Quickly check to see if we already have a database record for this thumbnail. We do this because predicting
+	// what the thumbnailer will generate is non-trivial, but it might generate a conflicting thumbnail (particularly
+	// when `defaultAnimated` is `true`.
+	db := database.GetInstance().Thumbnails.Prepare(ctx)
+	if res.i.Animated != animated { // this is the only thing that could have changed during generation
+		existingRecord, err := db.GetByParams(mediaRecord.Origin, mediaRecord.MediaId, width, height, method, res.i.Animated)
+		if err != nil {
+			return nil, nil, err
+		}
+		if existingRecord != nil {
+			ctx.Log.Debug("Found existing record for parameters - discarding generated thumbnail")
+			defer res.i.Reader.Close()
+
+			// Optimization: prevent future generator waste by inserting an `animated=true` record for static media,
+			// since we won't ever generate an animated version. This is safe because to get here the thumbnail needed
+			// to be requested as animated, but the generated one wasn't. This implies we are trying to animate a static
+			// image, which doesn't work.
+			if !res.i.Animated {
+				existingRecord.Animated = true
+				// we don't modify the creation time, so it expires at a sane point in history
+				err = db.Insert(existingRecord)
+				if err != nil {
+					ctx.Log.Warn("Non-fatal error while optimizing future thumbnail lookups: ", err)
+					sentry.CaptureException(err)
+				}
+			}
+
+			rsc, err := download.OpenStream(ctx, existingRecord.Locatable)
+			if err != nil {
+				return nil, nil, err
+			}
+			return existingRecord, rsc, nil
+		}
+	}
+
+	// We don't have an existing record. Store the stream and insert a record.
 	thumbMediaRecord, thumbStream, err := datastore_op.PutAndReturnStream(ctx, ctx.Request.Host, "", res.i.Reader, res.i.ContentType, "", datastores.ThumbnailsKind)
 	if err != nil {
 		return nil, nil, err
@@ -92,7 +129,7 @@ func Generate(ctx rcontext.RequestContext, mediaRecord *database.DbMedia, width
 			Location:    thumbMediaRecord.Location,
 		},
 	}
-	err = database.GetInstance().Thumbnails.Prepare(ctx).Insert(newRecord)
+	err = db.Insert(newRecord)
 	if err != nil {
 		defer thumbStream.Close()
 		return nil, nil, err

pipelines/pipeline_thumbnail/pipeline.go

@@ -77,12 +77,13 @@ func Execute(ctx rcontext.RequestContext, origin string, mediaId string, opts Th
 	r, err, _ := streamSf.Do(sfKey, func() (io.ReadCloser, error) {
 		// Step 4: Get the associated media record (without stream)
 		mediaRecord, dr, err := pipeline_download.Execute(ctx, origin, mediaId, opts.ImpliedDownloadOpts())
+		if dr != nil {
+			// Shouldn't be returned, but just in case...
+			dr.Close()
+		}
 		if err != nil {
 			if errors.Is(err, common.ErrMediaQuarantined) {
 				recordSf.OverwriteCacheKey(sfKey, nil) // force record to be nil (not found)
-				if dr != nil {
-					dr.Close()
-				}
 				return quarantine.ReturnAppropriateThing(ctx, false, opts.RecordOnly, opts.Width, opts.Height)
 			}
 			return nil, err

thumbnailing/thumbnail.go

@@ -50,9 +50,8 @@ func GenerateThumbnail(imgStream io.ReadCloser, contentType string, width int, h
 		return nil, common.ErrMediaTooLarge
 	}
 
-	// TODO: Why does AdjustProperties even take `canAnimate` if it's always been hardcoded to `false`? (see git blame on this comment)
 	var shouldThumbnail bool
-	shouldThumbnail, width, height, _, method = u.AdjustProperties(w, h, width, height, animated, false, method)
+	shouldThumbnail, width, height, _, method = u.AdjustProperties(w, h, width, height, animated, method)
 	if !shouldThumbnail && dimensional {
 		return nil, common.ErrMediaDimensionsTooSmall
 	}

thumbnailing/u/dimensions.go

 package u
 
-func AdjustProperties(srcWidth int, srcHeight int, desiredWidth int, desiredHeight int, wantAnimated bool, canAnimate bool, method string) (bool, int, int, bool, string) {
+func AdjustProperties(srcWidth int, srcHeight int, desiredWidth int, desiredHeight int, wantAnimated bool, method string) (bool, int, int, bool, string) {
 	aspectRatio := float32(srcHeight) / float32(srcWidth)
 	targetAspectRatio := float32(desiredHeight) / float32(desiredWidth)
 	if aspectRatio == targetAspectRatio {
@@ -11,8 +11,6 @@ func AdjustProperties(srcWidth int, srcHeight int, desiredWidth int, desiredHeig
 	if srcWidth <= desiredWidth && srcHeight <= desiredHeight {
 		if wantAnimated {
 			return true, srcWidth, srcHeight, true, method
-		} else if canAnimate {
-			return true, srcWidth, srcHeight, false, method
 		} else {
 			return false, desiredWidth, desiredHeight, false, method
 		}

util/sfcache/sfcache.go

@@ -25,7 +25,7 @@ func (c *SingleflightCache[T]) Do(key string, fn func() (T, error)) (T, error) {
 	}
 	var zero T
 	v, err, _ := c.sf.Do(key, fn)
-	if err != nil && v != zero {
+	if err == nil && v != zero {
 		c.cache.Store(key, v)
 	}
 	return v, err