fix(avatar): remove a replaced avatar's blob only after commit

Address review decision: SaveAvatar removed the old file's blob from
storage inside the transaction, so a later rollback left the user on an
avatar whose content was gone. The files row is still deleted in the
session; the blob and its previews go once the session commits.

pkg/files/files.go

@@ -28,6 +28,7 @@ import (
 
 	"code.vikunja.io/api/pkg/config"
 	"code.vikunja.io/api/pkg/db"
+	"code.vikunja.io/api/pkg/events"
 	"code.vikunja.io/api/pkg/log"
 
 	"code.vikunja.io/api/pkg/web"
@@ -206,7 +207,23 @@ func CreateWithMimeAndSession(s *xorm.Session, f io.ReadSeeker, realname string,
 
 // Delete removes a file from the DB and the file system.
 // The caller is responsible for committing or rolling back the session.
-func (f *File) Delete(s *xorm.Session) (err error) {
+func (f *File) Delete(s *xorm.Session) error {
+	if err := f.deleteRow(s); err != nil {
+		return err
+	}
+	return f.removeBlob()
+}
+
+// The blob goes in events.DispatchPending(s), so a rolled back session keeps it.
+func (f *File) DeleteOnCommit(s *xorm.Session) error {
+	if err := f.deleteRow(s); err != nil {
+		return err
+	}
+	events.OnCommit(s, f.removeBlob)
+	return nil
+}
+
+func (f *File) deleteRow(s *xorm.Session) error {
 	deleted, err := s.Where("id = ?", f.ID).Delete(&File{})
 	if err != nil {
 		return err
@@ -214,8 +231,11 @@ func (f *File) Delete(s *xorm.Session) (err error) {
 	if deleted == 0 {
 		return ErrFileDoesNotExist{FileID: f.ID}
 	}
+	return nil
+}
 
-	err = storage.Remove(f.fileID())
+func (f *File) removeBlob() error {
+	err := storage.Remove(f.fileID())
 	// After the file, so SavePreview catches a preview stored in between.
 	f.removePreviews()
 	if err != nil {

pkg/files/files_test.go

@@ -22,11 +22,13 @@ import (
 	"image"
 	"image/png"
 	"io"
+	"io/fs"
 	"net/http"
 	"testing"
 
 	"code.vikunja.io/api/pkg/config"
 	"code.vikunja.io/api/pkg/db"
+	"code.vikunja.io/api/pkg/events"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -169,6 +171,51 @@ func TestFile_Delete(t *testing.T) {
 	})
 }
 
+func TestFile_DeleteOnCommit(t *testing.T) {
+	t.Run("removes the blob and previews once dispatched", func(t *testing.T) {
+		s := db.NewSession()
+		defer s.Close()
+		initFixtures(t)
+		f := &File{ID: 1}
+		require.NoError(t, f.SavePreview(PreviewMedium, []byte("preview")))
+
+		require.NoError(t, f.DeleteOnCommit(s))
+		require.NoError(t, s.Commit())
+		db.AssertMissing(t, "files", map[string]interface{}{"id": 1})
+		_, err := FileStat(f)
+		require.NoError(t, err)
+
+		events.DispatchPending(t.Context(), s)
+		_, err = FileStat(f)
+		require.ErrorIs(t, err, fs.ErrNotExist)
+		_, _, err = f.OpenPreview(PreviewMedium)
+		require.ErrorIs(t, err, fs.ErrNotExist)
+	})
+	t.Run("keeps row and blob on rollback", func(t *testing.T) {
+		s := db.NewSession()
+		defer s.Close()
+		initFixtures(t)
+		f := &File{ID: 1}
+
+		require.NoError(t, f.DeleteOnCommit(s))
+		require.NoError(t, s.Rollback())
+		events.CleanupPending(s)
+		events.DispatchPending(t.Context(), s)
+
+		db.AssertExists(t, "files", map[string]interface{}{"id": 1}, false)
+		_, err := FileStat(f)
+		require.NoError(t, err)
+	})
+	t.Run("Nonexisting", func(t *testing.T) {
+		s := db.NewSession()
+		defer s.Close()
+		initFixtures(t)
+
+		err := (&File{ID: 9999}).DeleteOnCommit(s)
+		assert.True(t, IsErrFileDoesNotExist(err))
+	})
+}
+
 func TestFile_LoadFileByID(t *testing.T) {
 	t.Run("Normal", func(t *testing.T) {
 		initFixtures(t)

pkg/modules/avatar/avatar.go

@@ -25,6 +25,7 @@ import (
 
 	"code.vikunja.io/api/pkg/config"
 	"code.vikunja.io/api/pkg/db"
+	"code.vikunja.io/api/pkg/events"
 	"code.vikunja.io/api/pkg/log"
 	"code.vikunja.io/api/pkg/modules/avatar/botmarble"
 	"code.vikunja.io/api/pkg/modules/avatar/empty"
@@ -197,6 +198,7 @@ func SyncFromProvider(ctx context.Context, userID int64, provider string, raw []
 
 	s := db.NewSession()
 	defer s.Close()
+	defer events.CleanupPending(s)
 
 	u, err := user.GetUserByID(s, userID)
 	if err != nil {
@@ -212,6 +214,7 @@ func SyncFromProvider(ctx context.Context, userID int64, provider string, raw []
 	if err := s.Commit(); err != nil {
 		return fmt.Errorf("error committing avatar: %w", err)
 	}
+	events.DispatchPending(ctx, s)
 
 	FlushAllCaches(u)
 

pkg/modules/avatar/avatar_test.go

@@ -20,6 +20,7 @@ import (
 	"bytes"
 	"image"
 	"image/png"
+	"io/fs"
 	"testing"
 	"time"
 
@@ -100,6 +101,27 @@ func TestSyncFromProvider(t *testing.T) {
 		assert.Equal(t, 4, cfg.Height)
 	})
 
+	t.Run("removes the replaced avatar file and blob", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		files.InitTestFileFixtures(t)
+
+		setup := db.NewSession()
+		_, err := setup.ID(1).Cols("avatar_file_id", "avatar_provider").Update(&user.User{AvatarFileID: 1, AvatarProvider: "upload"})
+		require.NoError(t, err)
+		require.NoError(t, setup.Commit())
+		setup.Close()
+
+		_, err = files.FileStat(&files.File{ID: 1})
+		require.NoError(t, err)
+
+		require.NoError(t, SyncFromProvider(t.Context(), 1, "ldap", raw.Bytes()))
+
+		db.AssertMissing(t, "files", map[string]interface{}{"id": 1})
+		_, err = files.FileStat(&files.File{ID: 1})
+		require.Error(t, err)
+		assert.ErrorIs(t, err, fs.ErrNotExist)
+	})
+
 	t.Run("busy decoder leaves the avatar unchanged", func(t *testing.T) {
 		db.LoadAndAssertFixtures(t)
 		t.Cleanup(imageutils.ExhaustBudgetForTests(10 * time.Millisecond))

pkg/modules/avatar/upload/upload.go

@@ -158,6 +158,7 @@ func centeredSquare(img image.Image) image.Image {
 	return sub.SubImage(image.Rectangle{Min: topLeft, Max: topLeft.Add(image.Pt(side, side))})
 }
 
+// SaveAvatar removes the replaced avatar blob in events.DispatchPending(s).
 func SaveAvatar(s *xorm.Session, u *user.User, provider string, a *EncodedAvatar) (err error) {
 	oldAvatarFileID := u.AvatarFileID
 	oldProvider := u.AvatarProvider
@@ -189,7 +190,7 @@ func SaveAvatar(s *xorm.Session, u *user.User, provider string, a *EncodedAvatar
 	// Log only: the new avatar is already persisted.
 	if oldAvatarFileID != 0 {
 		old := &files.File{ID: oldAvatarFileID}
-		if delErr := old.Delete(s); delErr != nil && !files.IsErrFileDoesNotExist(delErr) {
+		if delErr := old.DeleteOnCommit(s); delErr != nil && !files.IsErrFileDoesNotExist(delErr) {
 			log.Errorf("Could not delete old avatar file %d of user %d: %s", oldAvatarFileID, u.ID, delErr)
 		}
 	}

pkg/modules/avatar/upload/upload_test.go

@@ -30,6 +30,7 @@ import (
 
 	"code.vikunja.io/api/pkg/config"
 	"code.vikunja.io/api/pkg/db"
+	"code.vikunja.io/api/pkg/events"
 	"code.vikunja.io/api/pkg/files"
 	"code.vikunja.io/api/pkg/log"
 	"code.vikunja.io/api/pkg/modules/imageutils"
@@ -221,10 +222,28 @@ func TestSaveAvatar(t *testing.T) {
 		db.AssertExists(t, "users", map[string]interface{}{"id": 1, "avatar_file_id": u.AvatarFileID}, false)
 		db.AssertMissing(t, "files", map[string]interface{}{"id": 1})
 		_, err := files.FileStat(&files.File{ID: 1})
+		require.NoError(t, err, "old avatar blob must survive until the commit is dispatched")
+
+		events.DispatchPending(t.Context(), s)
+		_, err = files.FileStat(&files.File{ID: 1})
 		require.Error(t, err, "old avatar blob must be gone")
 		assert.True(t, os.IsNotExist(err))
 	})
 
+	t.Run("keeps the old avatar when the session rolls back", func(t *testing.T) {
+		s, u := newUserWithAvatarFile(t)
+
+		require.NoError(t, SaveAvatar(s, u, "upload", prepare(t, encodePNG(t, gradient(2, 2)))))
+		require.NoError(t, s.Rollback())
+		events.CleanupPending(s)
+		events.DispatchPending(t.Context(), s)
+
+		db.AssertExists(t, "users", map[string]interface{}{"id": 1, "avatar_file_id": 1}, false)
+		db.AssertExists(t, "files", map[string]interface{}{"id": 1}, false)
+		_, err := files.FileStat(&files.File{ID: 1})
+		require.NoError(t, err, "old avatar blob must still exist")
+	})
+
 	t.Run("restores the user when the update fails", func(t *testing.T) {
 		s, u := newUserWithAvatarFile(t)
 		oldFileID, oldProvider := u.AvatarFileID, u.AvatarProvider

pkg/routes/api/v1/avatar.go

@@ -18,6 +18,7 @@ package v1
 
 import (
 	"code.vikunja.io/api/pkg/db"
+	"code.vikunja.io/api/pkg/events"
 	"code.vikunja.io/api/pkg/log"
 	"code.vikunja.io/api/pkg/models"
 	"code.vikunja.io/api/pkg/modules/avatar"
@@ -104,6 +105,7 @@ func UploadAvatar(c *echo.Context) (err error) {
 
 	s := db.NewSession()
 	defer s.Close()
+	defer events.CleanupPending(s)
 
 	u, err := user.GetUserByID(s, uc.ID)
 	if err != nil {
@@ -120,6 +122,7 @@ func UploadAvatar(c *echo.Context) (err error) {
 		_ = s.Rollback()
 		return err
 	}
+	events.DispatchPending(c.Request().Context(), s)
 
 	return c.JSON(http.StatusOK, models.Message{Message: "Avatar was uploaded successfully."})
 }

pkg/routes/api/v2/avatar_upload.go

@@ -22,6 +22,7 @@ import (
 	"net/http"
 
 	"code.vikunja.io/api/pkg/db"
+	"code.vikunja.io/api/pkg/events"
 	"code.vikunja.io/api/pkg/models"
 	"code.vikunja.io/api/pkg/modules/avatar"
 	"code.vikunja.io/api/pkg/user"
@@ -81,6 +82,7 @@ func avatarUpload(ctx context.Context, in *avatarUploadInput) (*avatarUploadBody
 
 	s := db.NewSession()
 	defer s.Close()
+	defer events.CleanupPending(s)
 
 	// Re-fetch the full user: the auth user from the JWT claims is partial.
 	u, err := user.GetUserByID(s, authUser.ID)
@@ -98,6 +100,7 @@ func avatarUpload(ctx context.Context, in *avatarUploadInput) (*avatarUploadBody
 		_ = s.Rollback()
 		return nil, translateDomainError(err)
 	}
+	events.DispatchPending(ctx, s)
 
 	return &avatarUploadBody{Body: &models.Message{Message: "Avatar was uploaded successfully."}}, nil
 }

pkg/webtests/huma_avatar_upload_test.go

@@ -25,6 +25,7 @@ import (
 	"image"
 	"image/color"
 	"image/png"
+	"io/fs"
 	"mime/multipart"
 	"net/http"
 	"net/http/httptest"
@@ -33,6 +34,7 @@ import (
 	"time"
 
 	"code.vikunja.io/api/pkg/db"
+	"code.vikunja.io/api/pkg/files"
 	"code.vikunja.io/api/pkg/modules/imageutils"
 	"code.vikunja.io/api/pkg/user"
 
@@ -215,6 +217,43 @@ func pngWithClaimedDimensionsPatch(t *testing.T, width, height int) []byte {
 	return data
 }
 
+func TestAvatarUploadRemovesReplacedFile(t *testing.T) {
+	for _, path := range []string{avatarUploadPath, "/api/v1/user/settings/avatar/upload"} {
+		t.Run(path, func(t *testing.T) {
+			e, err := setupTestEnv()
+			require.NoError(t, err)
+			token := humaTokenFor(t, &testuser1)
+
+			upload := func() int64 {
+				body, contentType := multipartAvatarBody(t, "avatar", "avatar.png", pngBytes(t))
+				req := httptest.NewRequest(http.MethodPut, path, body)
+				req.Header.Set("Content-Type", contentType)
+				req.Header.Set("Authorization", "Bearer "+token)
+				rec := httptest.NewRecorder()
+				e.ServeHTTP(rec, req)
+				require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String())
+
+				s := db.NewSession()
+				defer s.Close()
+				u, err := user.GetUserByID(s, testuser1.ID)
+				require.NoError(t, err)
+				return u.AvatarFileID
+			}
+
+			oldID := upload()
+			require.NotZero(t, oldID)
+			_, err = files.FileStat(&files.File{ID: oldID})
+			require.NoError(t, err)
+
+			newID := upload()
+			require.NotEqual(t, oldID, newID)
+			db.AssertMissing(t, "files", map[string]interface{}{"id": oldID})
+			_, err = files.FileStat(&files.File{ID: oldID})
+			require.ErrorIs(t, err, fs.ErrNotExist)
+		})
+	}
+}
+
 // Guards image allocation bounds (GHSA-4vh2-39rq-rq8j).
 func TestAvatarUploadImageBounds(t *testing.T) {
 	t.Run("8000 by 8000 avatar upload is rejected", func(t *testing.T) {
@@ -305,7 +344,11 @@ func TestAvatarDecodeIsBounded(t *testing.T) {
 			e.ServeHTTP(rec, req)
 		}()
 
-		<-ctx.waiting
+		select {
+		case <-ctx.waiting:
+		case <-time.After(5 * time.Second):
+			require.FailNow(t, "the upload never queued for decode capacity")
+		}
 		s := db.NewSession()
 		_, err = s.ID(testuser2.ID).Cols("name").Update(&user.User{Name: "written while an upload is queued"})
 		if err == nil {
@@ -319,7 +362,11 @@ func TestAvatarDecodeIsBounded(t *testing.T) {
 		}, false)
 
 		release()
-		<-finished
+		select {
+		case <-finished:
+		case <-time.After(10 * time.Second):
+			require.FailNow(t, "the upload did not finish after the budget was released")
+		}
 		assert.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String())
 	})