Some improvements

Author: lunny

models/fixtures/attachment.yml

@@ -99,7 +99,7 @@
   comment_id: 0
   name: attach1
   download_count: 0
-  size: 0
+  size: 29
   created_unix: 946684800
 
 -
@@ -153,16 +153,3 @@
   download_count: 0
   size: 0
   created_unix: 946684800
-
--
-  id: 13
-  uuid: a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a23
-  repo_id: 1
-  issue_id: 3
-  release_id: 0
-  uploader_id: 0
-  comment_id: 7
-  name: code_comment_uploaded_attachment.png
-  download_count: 0
-  size: 0
-  created_unix: 946684812

services/attachment/attachment_test.go

@@ -6,13 +6,16 @@ package attachment
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
+	"time"
 
 	"code.gitea.io/gitea/models/db"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/storage"
 	"code.gitea.io/gitea/services/storagecleanup"
 
 	_ "code.gitea.io/gitea/models/actions"
@@ -55,12 +58,26 @@ func TestUploadAttachment(t *testing.T) {
 func TestDeleteAttachments(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	attachment8 := unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 8})
+	const attachment8Content = "test content for attachment 8" // 29 bytes
+	_, err := storage.Attachments.Save(attachment8.RelativePath(), strings.NewReader(attachment8Content), int64(len(attachment8Content)))
+	assert.NoError(t, err)
+
+	fileInfo, err := storage.Attachments.Stat(attachment8.RelativePath())
+	assert.NoError(t, err)
+	assert.Equal(t, attachment8.Size, fileInfo.Size())
 
-	err := DeleteAttachment(db.DefaultContext, attachment8)
+	err = DeleteAttachment(db.DefaultContext, attachment8)
 	assert.NoError(t, err)
 
-	attachment, err := repo_model.GetAttachmentByUUID(db.DefaultContext, "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18")
+	attachment, err := repo_model.GetAttachmentByUUID(db.DefaultContext, attachment8.UUID)
 	assert.Error(t, err)
 	assert.True(t, repo_model.IsErrAttachmentNotExist(err))
 	assert.Nil(t, attachment)
+
+	// allow the queue to process the deletion
+	time.Sleep(1 * time.Second)
+
+	_, err = storage.Attachments.Stat(attachment8.RelativePath())
+	assert.Error(t, err)
+	assert.True(t, os.IsNotExist(err))
 }

services/issue/comments.go

@@ -141,7 +141,6 @@ func deleteComment(ctx context.Context, comment *issues_model.Comment, removeAtt
 			}
 		}
 
-		// deletedReviewComment should be a review comment with no content and no attachments
 		if err := issues_model.DeleteComment(ctx, comment); err != nil {
 			return nil, err
 		}

services/storagecleanup/storagecleanup.go

@@ -48,6 +48,7 @@ func cleanupDeletions(ctx context.Context, deletionIDs []int64) []int64 {
 		deletion, exist, err := db.GetByID[system.StoragePathDeletion](ctx, deletionID)
 		if err != nil {
 			log.Error("Failed to get deletion by ID %d: %v", deletionID, err)
+			failed = append(failed, deletionID)
 			continue
 		}
 		if !exist {
@@ -57,6 +58,7 @@ func cleanupDeletions(ctx context.Context, deletionIDs []int64) []int64 {
 		theStorage, err := storage.GetStorageByName(deletion.StorageName)
 		if err != nil {
 			log.Error("Failed to get storage by name %s: %v", deletion.StorageName, err)
+			failed = append(failed, deletionID)
 			continue
 		}
 		if err := theStorage.Delete(deletion.RelativePath); err != nil {
@@ -90,7 +92,6 @@ func ScanToBeDeletedFilesOrDir(ctx context.Context) error {
 	for {
 		if err := db.GetEngine(ctx).
 			Select("id").
-			// use the status and id index to speed up the query
 			Where("id > ?", lastID).
 			Asc("id").
 			Limit(100).

services/user/delete.go

@@ -100,12 +100,12 @@ func deleteUser(ctx context.Context, u *user_model.User, purge bool) (toBeCleane
 	}
 
 	if err := auth_model.DeleteOAuth2RelictsByUserID(ctx, u.ID); err != nil {
-		return nil, fmt.Errorf("deleteOAuth2RelictsByUserID: %w", err)
+		return nil, err
 	}
 
 	if purge || (setting.Service.UserDeleteWithCommentsMaxTime != 0 &&
 		u.CreatedUnix.AsTime().Add(setting.Service.UserDeleteWithCommentsMaxTime).After(time.Now())) {
-		// Delete Comments with attachments
+		// Delete Comments
 		const batchSize = 50
 		for {
 			comments := make([]*issues_model.Comment, 0, batchSize)