Alerting: Delete expired images from the database (#53236)

This commit adds a DeleteExpiredService that deletes expired images from the database. It is run in the periodic collector service.

pkg/server/wire.go

@@ -75,7 +75,9 @@ import (
 	authinfodatabase "github.com/grafana/grafana/pkg/services/login/authinfoservice/database"
 	"github.com/grafana/grafana/pkg/services/login/loginservice"
 	"github.com/grafana/grafana/pkg/services/ngalert"
+	ngimage "github.com/grafana/grafana/pkg/services/ngalert/image"
 	ngmetrics "github.com/grafana/grafana/pkg/services/ngalert/metrics"
+	ngstore "github.com/grafana/grafana/pkg/services/ngalert/store"
 	"github.com/grafana/grafana/pkg/services/notifications"
 	"github.com/grafana/grafana/pkg/services/oauthtoken"
 	"github.com/grafana/grafana/pkg/services/org/orgimpl"
@@ -211,6 +213,8 @@ var wireBasicSet = wire.NewSet(
 	contexthandler.ProvideService,
 	jwt.ProvideService,
 	wire.Bind(new(models.JWTService), new(*jwt.AuthService)),
+	ngstore.ProvideDBStore,
+	ngimage.ProvideDeleteExpiredService,
 	ngalert.ProvideService,
 	librarypanels.ProvideService,
 	wire.Bind(new(librarypanels.Service), new(*librarypanels.LibraryPanelService)),

pkg/services/cleanup/cleanup.go

@@ -8,31 +8,32 @@ import (
 	"path"
 	"time"
 
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/infra/serverlock"
+	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/annotations"
 	"github.com/grafana/grafana/pkg/services/dashboardsnapshots"
 	dashver "github.com/grafana/grafana/pkg/services/dashboardversion"
+	"github.com/grafana/grafana/pkg/services/ngalert/image"
 	"github.com/grafana/grafana/pkg/services/queryhistory"
 	"github.com/grafana/grafana/pkg/services/shorturls"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
-
-	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/infra/serverlock"
-	"github.com/grafana/grafana/pkg/models"
-	"github.com/grafana/grafana/pkg/services/annotations"
 	"github.com/grafana/grafana/pkg/setting"
 )
 
 func ProvideService(cfg *setting.Cfg, serverLockService *serverlock.ServerLockService,
-	shortURLService shorturls.Service, store sqlstore.Store, queryHistoryService queryhistory.Service,
-	dashboardVersionService dashver.Service, dashSnapSvc dashboardsnapshots.Service) *CleanUpService {
+	shortURLService shorturls.Service, sqlstore *sqlstore.SQLStore, queryHistoryService queryhistory.Service,
+	dashboardVersionService dashver.Service, dashSnapSvc dashboardsnapshots.Service, deleteExpiredImageService *image.DeleteExpiredService) *CleanUpService {
 	s := &CleanUpService{
 		Cfg:                       cfg,
 		ServerLockService:         serverLockService,
 		ShortURLService:           shortURLService,
 		QueryHistoryService:       queryHistoryService,
-		store:                    store,
+		store:                     sqlstore,
 		log:                       log.New("cleanup"),
 		dashboardVersionService:   dashboardVersionService,
 		dashboardSnapshotService:  dashSnapSvc,
+		deleteExpiredImageService: deleteExpiredImageService,
 	}
 	return s
 }
@@ -46,6 +47,7 @@ type CleanUpService struct {
 	QueryHistoryService       queryhistory.Service
 	dashboardVersionService   dashver.Service
 	dashboardSnapshotService  dashboardsnapshots.Service
+	deleteExpiredImageService *image.DeleteExpiredService
 }
 
 func (srv *CleanUpService) Run(ctx context.Context) error {
@@ -61,6 +63,7 @@ func (srv *CleanUpService) Run(ctx context.Context) error {
 			srv.cleanUpTmpFiles()
 			srv.deleteExpiredSnapshots(ctx)
 			srv.deleteExpiredDashboardVersions(ctx)
+			srv.deleteExpiredImages(ctx)
 			srv.cleanUpOldAnnotations(ctxWithTimeout)
 			srv.expireOldUserInvites(ctx)
 			srv.deleteStaleShortURLs(ctx)
@@ -156,6 +159,17 @@ func (srv *CleanUpService) deleteExpiredDashboardVersions(ctx context.Context) {
 	}
 }
 
+func (srv *CleanUpService) deleteExpiredImages(ctx context.Context) {
+	if !srv.Cfg.UnifiedAlerting.IsEnabled() {
+		return
+	}
+	if rowsAffected, err := srv.deleteExpiredImageService.DeleteExpired(ctx); err != nil {
+		srv.log.Error("Failed to delete expired images", "error", err.Error())
+	} else {
+		srv.log.Debug("Deleted expired images", "rows affected", rowsAffected)
+	}
+}
+
 func (srv *CleanUpService) deleteOldLoginAttempts(ctx context.Context) {
 	if srv.Cfg.DisableBruteForceLoginProtection {
 		return

pkg/services/ngalert/image/service.go

@@ -32,6 +32,19 @@ var (
 	ErrNoPanel = errors.New("no panel")
 )
 
+// DeleteExpiredService is a service to delete expired images.
+type DeleteExpiredService struct {
+	store store.ImageAdminStore
+}
+
+func (s *DeleteExpiredService) DeleteExpired(ctx context.Context) (int64, error) {
+	return s.store.DeleteExpiredImages(ctx)
+}
+
+func ProvideDeleteExpiredService(store *store.DBstore) *DeleteExpiredService {
+	return &DeleteExpiredService{store: store}
+}
+
 //go:generate mockgen -destination=mock.go -package=image github.com/grafana/grafana/pkg/services/ngalert/image ImageService
 type ImageService interface {
 	// NewImage returns a new image for the alert instance.
@@ -127,14 +140,16 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *ngmodels.Alert
 	return &v, nil
 }
 
+// NotAvailableImageService is a service that returns ErrScreenshotsUnavailable.
 type NotAvailableImageService struct{}
 
-func (s *NotAvailableImageService) NewImage(ctx context.Context, r *ngmodels.AlertRule) (*ngmodels.Image, error) {
+func (s *NotAvailableImageService) NewImage(_ context.Context, _ *ngmodels.AlertRule) (*ngmodels.Image, error) {
 	return nil, screenshot.ErrScreenshotsUnavailable
 }
 
+// NoopImageService is a no-op image service.
 type NoopImageService struct{}
 
-func (s *NoopImageService) NewImage(ctx context.Context, r *ngmodels.AlertRule) (*ngmodels.Image, error) {
+func (s *NoopImageService) NewImage(_ context.Context, _ *ngmodels.AlertRule) (*ngmodels.Image, error) {
 	return &ngmodels.Image{}, nil
 }

pkg/services/ngalert/models/image.go

@@ -5,8 +5,10 @@ import (
 	"time"
 )
 
+var (
 	// ErrImageNotFound is returned when the image does not exist.
-var ErrImageNotFound = errors.New("image not found")
+	ErrImageNotFound = errors.New("image not found")
+)
 
 type Image struct {
 	ID        int64     `xorm:"pk autoincr 'id'"`
@@ -17,6 +19,27 @@ type Image struct {
 	ExpiresAt time.Time `xorm:"expires_at"`
 }
 
+// ExtendDuration extends the expiration time of the image. It can shorten
+// the duration of the image if d is negative.
+func (i *Image) ExtendDuration(d time.Duration) {
+	i.ExpiresAt = i.ExpiresAt.Add(d)
+}
+
+// HasExpired returns true if the image has expired.
+func (i *Image) HasExpired() bool {
+	return time.Now().After(i.ExpiresAt)
+}
+
+// HasPath returns true if the image has a path on disk.
+func (i *Image) HasPath() bool {
+	return i.Path != ""
+}
+
+// HasURL returns true if the image has a URL.
+func (i *Image) HasURL() bool {
+	return i.URL != ""
+}
+
 // A XORM interface that defines the used table for this struct.
 func (i *Image) TableName() string {
 	return "alert_image"

pkg/services/ngalert/models/image_test.go

@@ -0,0 +1,48 @@
+package models
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestImage_ExtendDuration(t *testing.T) {
+	var i Image
+	d := time.Now().Add(time.Minute)
+	i.ExpiresAt = d
+	// extend the duration for 1 minute
+	i.ExtendDuration(time.Minute)
+	assert.Equal(t, d.Add(time.Minute), i.ExpiresAt)
+	// can shorten the duration too
+	i.ExtendDuration(-time.Minute)
+	assert.Equal(t, d, i.ExpiresAt)
+}
+
+func TestImage_HasExpired(t *testing.T) {
+	var i Image
+	i.ExpiresAt = time.Now().Add(time.Minute)
+	assert.False(t, i.HasExpired())
+	i.ExpiresAt = time.Now()
+	assert.True(t, i.HasExpired())
+	i.ExpiresAt = time.Now().Add(-time.Minute)
+	assert.True(t, i.HasExpired())
+}
+
+func TestImage_HasPath(t *testing.T) {
+	var i Image
+	assert.False(t, i.HasPath())
+	i.Path = "/"
+	assert.True(t, i.HasPath())
+	i.Path = "/tmp/image.png"
+	assert.True(t, i.HasPath())
+}
+
+func TestImage_HasURL(t *testing.T) {
+	var i Image
+	assert.False(t, i.HasURL())
+	i.URL = "/"
+	assert.True(t, i.HasURL())
+	i.URL = "https://example.com/image.png"
+	assert.True(t, i.HasURL())
+}

pkg/services/ngalert/notifier/testing.go

@@ -27,8 +27,8 @@ func (f *FakeConfigStore) GetImage(ctx context.Context, token string) (*models.I
 	return nil, models.ErrImageNotFound
 }
 
-func (f *FakeConfigStore) GetImages(ctx context.Context, tokens []string) ([]models.Image, error) {
-	return nil, models.ErrImageNotFound
+func (f *FakeConfigStore) GetImages(ctx context.Context, tokens []string) ([]models.Image, []string, error) {
+	return nil, nil, models.ErrImageNotFound
 }
 
 func NewFakeConfigStore(t *testing.T, configs map[int64]*models.AlertConfiguration) FakeConfigStore {

pkg/services/ngalert/store/database.go

@@ -36,3 +36,16 @@ type DBstore struct {
 	AccessControl    accesscontrol.AccessControl
 	DashboardService dashboards.DashboardService
 }
+
+func ProvideDBStore(
+	cfg *setting.Cfg, sqlstore *sqlstore.SQLStore, folderService dashboards.FolderService,
+	access accesscontrol.AccessControl, dashboards dashboards.DashboardService) *DBstore {
+	return &DBstore{
+		Cfg:              cfg.UnifiedAlerting,
+		SQLStore:         sqlstore,
+		Logger:           log.New("dbstore"),
+		FolderService:    folderService,
+		AccessControl:    access,
+		DashboardService: dashboards,
+	}
+}

pkg/services/ngalert/store/image.go

@@ -11,84 +11,123 @@ import (
 	"github.com/grafana/grafana/pkg/services/sqlstore"
 )
 
+const (
+	imageExpirationDuration = 24 * time.Hour
+)
+
 type ImageStore interface {
-	// GetImage returns the image with the token or ErrImageNotFound.
+	// GetImage returns the image with the token. It returns ErrImageNotFound
+	// if the image has expired or if an image with the token does not exist.
 	GetImage(ctx context.Context, token string) (*models.Image, error)
 
-	// GetImages returns all images that match the tokens. If one or more
-	// tokens does not exist then it also returns ErrImageNotFound.
-	GetImages(ctx context.Context, tokens []string) ([]models.Image, error)
+	// GetImages returns all images that match the tokens. If one or more images
+	// have expired or do not exist then it also returns the unmatched tokens
+	// and an ErrImageNotFound error.
+	GetImages(ctx context.Context, tokens []string) ([]models.Image, []string, error)
 
 	// SaveImage saves the image or returns an error.
 	SaveImage(ctx context.Context, img *models.Image) error
 }
 
+type ImageAdminStore interface {
+	ImageStore
+
+	// DeleteExpiredImages deletes expired images. It returns the number of deleted images
+	// or an error.
+	DeleteExpiredImages(context.Context) (int64, error)
+}
+
 func (st DBstore) GetImage(ctx context.Context, token string) (*models.Image, error) {
-	var img models.Image
+	var image models.Image
 	if err := st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
-		exists, err := sess.Where("token = ?", token).Get(&img)
+		exists, err := sess.Where("token = ? AND expires_at > ?", token, TimeNow().UTC()).Get(&image)
 		if err != nil {
 			return fmt.Errorf("failed to get image: %w", err)
-		}
-		if !exists {
+		} else if !exists {
 			return models.ErrImageNotFound
-		}
+		} else {
 			return nil
+		}
 	}); err != nil {
 		return nil, err
 	}
-	return &img, nil
+	return &image, nil
 }
 
-func (st DBstore) GetImages(ctx context.Context, tokens []string) ([]models.Image, error) {
-	var imgs []models.Image
+func (st DBstore) GetImages(ctx context.Context, tokens []string) ([]models.Image, []string, error) {
+	var images []models.Image
 	if err := st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
-		return sess.In("token", tokens).Find(&imgs)
+		return sess.In("token", tokens).Where("expires_at > ?", TimeNow().UTC()).Find(&images)
 	}); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
-	if len(imgs) < len(tokens) {
-		return imgs, models.ErrImageNotFound
+	if len(images) < len(tokens) {
+		return images, unmatchedTokens(tokens, images), models.ErrImageNotFound
 	}
-	return imgs, nil
+	return images, nil, nil
 }
 
 func (st DBstore) SaveImage(ctx context.Context, img *models.Image) error {
 	return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
-		// TODO: Is this a good idea? Do we actually want to automatically expire
-		// rows? See issue https://github.com/grafana/grafana/issues/49366
-		img.ExpiresAt = TimeNow().Add(1 * time.Minute).UTC()
-		if img.ID == 0 { // xorm will fill this field on Insert.
+		if img.ID == 0 {
+			// If the ID is zero then this is a new image. It needs a token, a created timestamp
+			// and an expiration time. The expiration time of the image is derived from the created
+			// timestamp rather than the current time as it helps assert that the expiration time
+			// has the intended duration in tests.
 			token, err := uuid.NewRandom()
 			if err != nil {
 				return fmt.Errorf("failed to create token: %w", err)
 			}
 			img.Token = token.String()
 			img.CreatedAt = TimeNow().UTC()
+			img.ExpiresAt = img.CreatedAt.Add(imageExpirationDuration)
 			if _, err := sess.Insert(img); err != nil {
-				return fmt.Errorf("failed to insert screenshot: %w", err)
+				return fmt.Errorf("failed to insert image: %w", err)
 			}
 		} else {
-			affected, err := sess.ID(img.ID).Update(img)
-			if err != nil {
-				return fmt.Errorf("failed to update screenshot: %v", err)
+			// Check if the image exists as some databases return 0 rows affected if
+			// no changes were made
+			if ok, err := sess.Where("id = ?", img.ID).ForUpdate().Exist(&models.Image{}); err != nil {
+				return fmt.Errorf("failed to check if image exists: %v", err)
+			} else if !ok {
+				return models.ErrImageNotFound
 			}
-			if affected == 0 {
-				return fmt.Errorf("update statement had no effect")
+
+			// Do not reset the expiration time as it can be extended with ExtendDuration
+			if _, err := sess.ID(img.ID).Update(img); err != nil {
+				return fmt.Errorf("failed to update image: %v", err)
 			}
 		}
 		return nil
 	})
 }
 
-//nolint:unused
-func (st DBstore) DeleteExpiredImages(ctx context.Context) error {
-	return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
-		n, err := sess.Where("expires_at < ?", TimeNow()).Delete(&models.Image{})
+func (st DBstore) DeleteExpiredImages(ctx context.Context) (int64, error) {
+	var n int64
+	if err := st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
+		rows, err := sess.Where("expires_at < ?", TimeNow().UTC()).Delete(&models.Image{})
 		if err != nil {
 			return fmt.Errorf("failed to delete expired images: %w", err)
 		}
-		st.Logger.Info("deleted expired images", "n", n)
-		return err
-	})
+		n = rows
+		return nil
+	}); err != nil {
+		return -1, err
+	}
+	return n, nil
+}
+
+// unmatchedTokens returns the tokens that were not matched to an image.
+func unmatchedTokens(tokens []string, images []models.Image) []string {
+	matched := make(map[string]struct{})
+	for _, image := range images {
+		matched[image.Token] = struct{}{}
+	}
+	unmatched := make([]string, 0, len(tokens))
+	for _, token := range tokens {
+		if _, ok := matched[token]; !ok {
+			unmatched = append(unmatched, token)
+		}
+	}
+	return unmatched
 }

pkg/services/ngalert/store/image_test.go

@@ -5,7 +5,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -14,182 +13,175 @@ import (
 	"github.com/grafana/grafana/pkg/services/ngalert/tests"
 )
 
-func createTestImg(fakeUrl string, fakePath string) *models.Image {
-	return &models.Image{
-		ID:    0,
-		Token: "",
-		Path:  fakeUrl + "local",
-		URL:   fakeUrl,
-	}
-}
-
-func addID(img *models.Image, id int64) *models.Image {
-	img.ID = id
-	return img
-}
-
-func addToken(img *models.Image) *models.Image {
-	token, err := uuid.NewRandom()
-	if err != nil {
-		panic("wat")
-	}
-	img.Token = token.String()
-	return img
-}
-
 func TestIntegrationSaveAndGetImage(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test")
 	}
-	mockTimeNow()
+
+	// our database schema uses second precision for timestamps
+	store.TimeNow = func() time.Time {
+		return time.Now().Truncate(time.Second)
+	}
+
 	ctx := context.Background()
 	_, dbstore := tests.SetupTestEnv(t, baseIntervalSeconds)
 
-	// Here are some images to save.
-	imgs := []struct {
-		name   string
-		img    *models.Image
-		errors bool
-	}{
-		{
-			"with file path",
-			createTestImg("", "path"),
-			false,
-		},
-		{
-			"with URL",
-			createTestImg("url", ""),
-			false,
-		},
-		{
-			"ID already set, should not change",
-			addToken(addID(createTestImg("Foo", ""), 123)),
-			true,
-		},
-	}
+	// create an image with a path on disk
+	image1 := models.Image{Path: "example.png"}
+	require.NoError(t, dbstore.SaveImage(ctx, &image1))
+	require.NotEqual(t, "", image1.Token)
 
-	for _, test := range imgs {
-		t.Run(test.name, func(t *testing.T) {
-			ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
-			defer cancel()
-			err := dbstore.SaveImage(ctx, test.img)
-			if test.errors {
-				require.Error(t, err)
-				return
-			}
+	// image should not have expired
+	assert.False(t, image1.HasExpired())
+	assert.Equal(t, image1.ExpiresAt, image1.CreatedAt.Add(24*time.Hour))
 
+	// should return the image with a path on disk
+	result1, err := dbstore.GetImage(ctx, image1.Token)
 	require.NoError(t, err)
-			returned, err := dbstore.GetImage(ctx, test.img.Token)
-			assert.NoError(t, err, "Shouldn't error when getting the image")
-			assert.Equal(t, test.img, returned)
-
-			// Save again to test update path.
-			err = dbstore.SaveImage(ctx, test.img)
-			require.NoError(t, err, "Should have no error on second write")
-			returned, err = dbstore.GetImage(ctx, test.img.Token)
-			assert.NoError(t, err, "Shouldn't error when getting the image a second time")
-			assert.Equal(t, test.img, returned)
-		})
-	}
+	assert.Equal(t, image1, *result1)
+
+	// save the image a second time should not change the expiration time
+	ts := image1.ExpiresAt
+	require.NoError(t, dbstore.SaveImage(ctx, &image1))
+	assert.Equal(t, image1.ExpiresAt, ts)
+
+	// create an image with a URL
+	image2 := models.Image{URL: "https://example.com/example.png"}
+	require.NoError(t, dbstore.SaveImage(ctx, &image2))
+	require.NotEqual(t, "", image2.Token)
+
+	// image should not have expired
+	assert.False(t, image2.HasExpired())
+	assert.Equal(t, image2.ExpiresAt, image2.CreatedAt.Add(24*time.Hour))
+
+	// should return the image with a URL
+	result2, err := dbstore.GetImage(ctx, image2.Token)
+	require.NoError(t, err)
+	assert.Equal(t, image2, *result2)
+
+	// expired image should not be returned
+	image1.ExpiresAt = time.Now().Add(-time.Second)
+	require.NoError(t, dbstore.SaveImage(ctx, &image1))
+	result1, err = dbstore.GetImage(ctx, image1.Token)
+	assert.EqualError(t, err, "image not found")
+	assert.Nil(t, result1)
 }
 
 func TestIntegrationGetImages(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test")
 	}
-	mockTimeNow()
-	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-	defer cancel()
+
+	// our database schema uses second precision for timestamps
+	store.TimeNow = func() time.Time {
+		return time.Now().Truncate(time.Second)
+	}
+
+	ctx := context.Background()
 	_, dbstore := tests.SetupTestEnv(t, baseIntervalSeconds)
 
-	// create an image foo.png
-	img1 := models.Image{Path: "foo.png"}
-	require.NoError(t, dbstore.SaveImage(ctx, &img1))
+	// create an image with a path on disk
+	image1 := models.Image{Path: "example.png"}
+	require.NoError(t, dbstore.SaveImage(ctx, &image1))
 
-	// GetImages should return the first image
-	imgs, err := dbstore.GetImages(ctx, []string{img1.Token})
+	// should return the first image
+	images, mismatched, err := dbstore.GetImages(ctx, []string{image1.Token})
 	require.NoError(t, err)
-	assert.Equal(t, []models.Image{img1}, imgs)
+	assert.Len(t, mismatched, 0)
+	assert.Equal(t, []models.Image{image1}, images)
 
-	// create another image bar.png
-	img2 := models.Image{Path: "bar.png"}
-	require.NoError(t, dbstore.SaveImage(ctx, &img2))
+	// create an image with a URL
+	image2 := models.Image{Path: "https://example.com/example.png"}
+	require.NoError(t, dbstore.SaveImage(ctx, &image2))
 
-	// GetImages should return both images
-	imgs, err = dbstore.GetImages(ctx, []string{img1.Token, img2.Token})
+	// should return both images
+	images, mismatched, err = dbstore.GetImages(ctx, []string{image1.Token, image2.Token})
 	require.NoError(t, err)
-	assert.ElementsMatch(t, []models.Image{img1, img2}, imgs)
+	assert.Len(t, mismatched, 0)
+	assert.ElementsMatch(t, []models.Image{image1, image2}, images)
 
-	// GetImages should return the first image
-	imgs, err = dbstore.GetImages(ctx, []string{img1.Token})
+	// should return the first image
+	images, mismatched, err = dbstore.GetImages(ctx, []string{image1.Token})
 	require.NoError(t, err)
-	assert.Equal(t, []models.Image{img1}, imgs)
+	assert.Len(t, mismatched, 0)
+	assert.Equal(t, []models.Image{image1}, images)
 
-	// GetImages should return the second image
-	imgs, err = dbstore.GetImages(ctx, []string{img2.Token})
+	// should return the second image
+	images, mismatched, err = dbstore.GetImages(ctx, []string{image2.Token})
 	require.NoError(t, err)
-	assert.Equal(t, []models.Image{img2}, imgs)
+	assert.Len(t, mismatched, 0)
+	assert.Equal(t, []models.Image{image2}, images)
 
-	// GetImages should return the first image and an error
-	imgs, err = dbstore.GetImages(ctx, []string{img1.Token, "unknown"})
+	// should return the first image and an error
+	images, mismatched, err = dbstore.GetImages(ctx, []string{image1.Token, "unknown"})
 	assert.EqualError(t, err, "image not found")
-	assert.Equal(t, []models.Image{img1}, imgs)
+	assert.Equal(t, []string{"unknown"}, mismatched)
+	assert.Equal(t, []models.Image{image1}, images)
 
-	// GetImages should return no images for no tokens
-	imgs, err = dbstore.GetImages(ctx, []string{})
+	// should return no images for no tokens
+	images, mismatched, err = dbstore.GetImages(ctx, []string{})
 	require.NoError(t, err)
-	assert.Len(t, imgs, 0)
+	assert.Len(t, mismatched, 0)
+	assert.Len(t, images, 0)
 
-	// GetImages should return no images for nil tokens
-	imgs, err = dbstore.GetImages(ctx, nil)
+	// should return no images for nil tokens
+	images, mismatched, err = dbstore.GetImages(ctx, nil)
 	require.NoError(t, err)
-	assert.Len(t, imgs, 0)
+	assert.Len(t, mismatched, 0)
+	assert.Len(t, images, 0)
+
+	// expired image should not be returned
+	image1.ExpiresAt = time.Now().Add(-time.Second)
+	require.NoError(t, dbstore.SaveImage(ctx, &image1))
+	images, mismatched, err = dbstore.GetImages(ctx, []string{image1.Token, image2.Token})
+	assert.EqualError(t, err, "image not found")
+	assert.Equal(t, []string{image1.Token}, mismatched)
+	assert.Equal(t, []models.Image{image2}, images)
 }
 
 func TestIntegrationDeleteExpiredImages(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test")
 	}
-	mockTimeNow()
-	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
-	defer cancel()
-	_, dbstore := tests.SetupTestEnv(t, baseIntervalSeconds)
-
-	// Save two images.
-	imgs := []*models.Image{
-		createTestImg("", ""),
-		createTestImg("", ""),
-	}
 
-	for _, img := range imgs {
-		err := dbstore.SaveImage(ctx, img)
-		require.NoError(t, err)
+	// our database schema uses second precision for timestamps
+	store.TimeNow = func() time.Time {
+		return time.Now().Truncate(time.Second)
 	}
 
-	// Images are availabile
-	img, err := dbstore.GetImage(ctx, imgs[0].Token)
-	require.NoError(t, err)
-	require.NotNil(t, img)
+	ctx := context.Background()
+	_, dbstore := tests.SetupTestEnv(t, baseIntervalSeconds)
 
-	img, err = dbstore.GetImage(ctx, imgs[1].Token)
-	require.NoError(t, err)
-	require.NotNil(t, img)
+	// create two images
+	image1 := models.Image{Path: "example.png"}
+	require.NoError(t, dbstore.SaveImage(ctx, &image1))
+	image2 := models.Image{URL: "https://example.com/example.png"}
+	require.NoError(t, dbstore.SaveImage(ctx, &image2))
 
-	// Wait until timeout.
-	for i := 0; i < 120; i++ {
-		store.TimeNow()
-	}
+	s := dbstore.SQLStore.NewSession(ctx)
+	t.Cleanup(s.Close)
 
-	// Call expired
-	err = dbstore.DeleteExpiredImages(ctx)
+	// should return both images
+	var result1, result2 models.Image
+	ok, err := s.Where("token = ?", image1.Token).Get(&result1)
+	require.NoError(t, err)
+	assert.True(t, ok)
+	ok, err = s.Where("token = ?", image2.Token).Get(&result2)
 	require.NoError(t, err)
+	assert.True(t, ok)
 
-	// All images are gone.
-	img, err = dbstore.GetImage(ctx, imgs[0].Token)
-	require.Nil(t, img)
-	require.Error(t, err)
+	// should delete expired image
+	image1.ExpiresAt = time.Now().Add(-time.Second)
+	require.NoError(t, dbstore.SaveImage(ctx, &image1))
+	n, err := dbstore.DeleteExpiredImages(ctx)
+	require.NoError(t, err)
+	assert.Equal(t, int64(1), n)
 
-	img, err = dbstore.GetImage(ctx, imgs[1].Token)
-	require.Nil(t, img)
-	require.Error(t, err)
+	// should return just the second image
+	ok, err = s.Where("token = ?", image1.Token).Get(&result1)
+	require.NoError(t, err)
+	assert.False(t, ok)
+	ok, err = s.Where("token = ?", image2.Token).Get(&result2)
+	require.NoError(t, err)
+	assert.True(t, ok)
 }

pkg/services/ngalert/store/instance_database_test.go

@@ -3,27 +3,15 @@ package store_test
 import (
 	"context"
 	"testing"
-	"time"
+
+	"github.com/stretchr/testify/require"
 
 	"github.com/grafana/grafana/pkg/services/ngalert/models"
-	"github.com/grafana/grafana/pkg/services/ngalert/store"
 	"github.com/grafana/grafana/pkg/services/ngalert/tests"
-
-	"github.com/stretchr/testify/require"
 )
 
 const baseIntervalSeconds = 10
 
-// Every time this is called, time advances by 1 second.
-func mockTimeNow() {
-	var timeSeed int64
-	store.TimeNow = func() time.Time {
-		fakeNow := time.Unix(timeSeed, 0).UTC()
-		timeSeed++
-		return fakeNow
-	}
-}
-
 func TestIntegrationAlertInstanceOperations(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test")