Improve attachments deletion

models/fixtures/attachment.yml

@@ -99,7 +99,7 @@
   comment_id: 0
   name: attach1
   download_count: 0
-  size: 0
+  size: 29
   created_unix: 946684800
 
 -

models/issues/comment.go

@@ -390,18 +390,6 @@ func (c *Comment) LoadPoster(ctx context.Context) (err error) {
 	return err
 }
 
-// AfterDelete is invoked from XORM after the object is deleted.
-func (c *Comment) AfterDelete(ctx context.Context) {
-	if c.ID <= 0 {
-		return
-	}
-
-	_, err := repo_model.DeleteAttachmentsByComment(ctx, c.ID, true)
-	if err != nil {
-		log.Info("Could not delete files for comment %d on issue #%d: %s", c.ID, c.IssueID, err)
-	}
-}
-
 // HTMLURL formats a URL-string to the issue-comment
 func (c *Comment) HTMLURL(ctx context.Context) string {
 	err := c.LoadIssue(ctx)
@@ -611,6 +599,11 @@ func UpdateCommentAttachments(ctx context.Context, c *Comment, uuids []string) e
 			return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err)
 		}
 		for i := range attachments {
+			if attachments[i].CommentID == c.ID && attachments[i].IssueID == c.IssueID {
+				continue
+			} else if attachments[i].IssueID != 0 || attachments[i].CommentID != 0 {
+				return util.NewPermissionDeniedErrorf("update comment attachments permission denied")
+			}
 			attachments[i].IssueID = c.IssueID
 			attachments[i].CommentID = c.ID
 			if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil {

models/issues/issue_update.go

@@ -305,6 +305,11 @@ func UpdateIssueAttachments(ctx context.Context, issueID int64, uuids []string)
 		return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", uuids, err)
 	}
 	for i := range attachments {
+		if attachments[i].IssueID == issueID {
+			continue
+		} else if attachments[i].IssueID != 0 {
+			return util.NewPermissionDeniedErrorf("update issue attachments permission denied")
+		}
 		attachments[i].IssueID = issueID
 		if err := repo_model.UpdateAttachment(ctx, attachments[i]); err != nil {
 			return fmt.Errorf("update attachment [id: %d]: %w", attachments[i].ID, err)

models/migrations/migrations.go

@@ -24,6 +24,7 @@ import (
 	"code.gitea.io/gitea/models/migrations/v1_22"
 	"code.gitea.io/gitea/models/migrations/v1_23"
 	"code.gitea.io/gitea/models/migrations/v1_24"
+	"code.gitea.io/gitea/models/migrations/v1_25"
 	"code.gitea.io/gitea/models/migrations/v1_6"
 	"code.gitea.io/gitea/models/migrations/v1_7"
 	"code.gitea.io/gitea/models/migrations/v1_8"
@@ -382,6 +383,9 @@ func prepareMigrationTasks() []*migration {
 		newMigration(318, "Add anonymous_access_mode for repo_unit", v1_24.AddRepoUnitAnonymousAccessMode),
 		newMigration(319, "Add ExclusiveOrder to Label table", v1_24.AddExclusiveOrderColumnToLabelTable),
 		newMigration(320, "Migrate two_factor_policy to login_source table", v1_24.MigrateSkipTwoFactor),
+
+		// Gitea 1.24.0-rc0 ends at migration ID number 320 (database version 321)
+		newMigration(321, "Add storage_path_deletion table", v1_25.AddStoragePathDeletion),
 	}
 	return preparedMigrations
 }

models/migrations/v1_25/v321.go

@@ -0,0 +1,26 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package v1_25
+
+import (
+	"code.gitea.io/gitea/modules/timeutil"
+
+	"xorm.io/xorm"
+)
+
+func AddStoragePathDeletion(x *xorm.Engine) error {
+	// StoragePathDeletion represents a file or directory that is pending deletion.
+	type StoragePathDeletion struct {
+		ID                     int64
+		StorageName            string             // storage name defines in storage module
+		PathType               int                // 1 for file, 2 for directory
+		RelativePath           string             `xorm:"TEXT"`
+		DeleteFailedCount      int                `xorm:"DEFAULT 0 NOT NULL"` // Number of times the deletion failed, used to prevent infinite loop
+		LastDeleteFailedReason string             `xorm:"TEXT"`               // Last reason the deletion failed, used to prevent infinite loop
+		LastDeleteFailedTime   timeutil.TimeStamp // Last time the deletion failed, used to prevent infinite loop
+		CreatedUnix            timeutil.TimeStamp `xorm:"INDEX created"`
+	}
+
+	return x.Sync(new(StoragePathDeletion))
+}

models/repo/attachment.go

@@ -8,11 +8,10 @@ import (
 	"errors"
 	"fmt"
 	"net/url"
-	"os"
 	"path"
 
 	"code.gitea.io/gitea/models/db"
-	"code.gitea.io/gitea/modules/log"
+	system_model "code.gitea.io/gitea/models/system"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/storage"
 	"code.gitea.io/gitea/modules/timeutil"
@@ -166,59 +165,39 @@ func GetAttachmentByReleaseIDFileName(ctx context.Context, releaseID int64, file
 	return attach, nil
 }
 
-// DeleteAttachment deletes the given attachment and optionally the associated file.
-func DeleteAttachment(ctx context.Context, a *Attachment, remove bool) error {
-	_, err := DeleteAttachments(ctx, []*Attachment{a}, remove)
-	return err
-}
-
-// DeleteAttachments deletes the given attachments and optionally the associated files.
-func DeleteAttachments(ctx context.Context, attachments []*Attachment, remove bool) (int, error) {
+// DeleteAttachments delete the given attachments and add disk files to pending deletion
+func DeleteAttachments(ctx context.Context, attachments []*Attachment) ([]int64, error) {
 	if len(attachments) == 0 {
-		return 0, nil
+		return nil, nil
 	}
 
 	ids := make([]int64, 0, len(attachments))
 	for _, a := range attachments {
 		ids = append(ids, a.ID)
 	}
 
-	cnt, err := db.GetEngine(ctx).In("id", ids).NoAutoCondition().Delete(attachments[0])
-	if err != nil {
-		return 0, err
-	}
-
-	if remove {
-		for i, a := range attachments {
-			if err := storage.Attachments.Delete(a.RelativePath()); err != nil {
-				if !errors.Is(err, os.ErrNotExist) {
-					return i, err
-				}
-				log.Warn("Attachment file not found when deleting: %s", a.RelativePath())
-			}
+	return db.WithTx2(ctx, func(ctx context.Context) ([]int64, error) {
+		// delete attachments from database
+		if _, err := db.GetEngine(ctx).Table("attachment").In("id", ids).Delete(); err != nil {
+			return nil, err
 		}
-	}
-	return int(cnt), nil
-}
-
-// DeleteAttachmentsByIssue deletes all attachments associated with the given issue.
-func DeleteAttachmentsByIssue(ctx context.Context, issueID int64, remove bool) (int, error) {
-	attachments, err := GetAttachmentsByIssueID(ctx, issueID)
-	if err != nil {
-		return 0, err
-	}
-
-	return DeleteAttachments(ctx, attachments, remove)
-}
 
-// DeleteAttachmentsByComment deletes all attachments associated with the given comment.
-func DeleteAttachmentsByComment(ctx context.Context, commentID int64, remove bool) (int, error) {
-	attachments, err := GetAttachmentsByCommentID(ctx, commentID)
-	if err != nil {
-		return 0, err
-	}
+		// add disk files to pending deletion table as well
+		var deletionIDs []int64
+		for _, a := range attachments {
+			pendingDeletion := &system_model.StoragePathDeletion{
+				StorageName:  storage.AttachmentStorageName,
+				PathType:     system_model.PathFile,
+				RelativePath: a.RelativePath(),
+			}
+			if err := db.Insert(ctx, pendingDeletion); err != nil {
+				return nil, fmt.Errorf("insert pending deletion: %w", err)
+			}
 
-	return DeleteAttachments(ctx, attachments, remove)
+			deletionIDs = append(deletionIDs, pendingDeletion.ID) // Collect pending deletions
+		}
+		return deletionIDs, nil
+	})
 }
 
 // UpdateAttachmentByUUID Updates attachment via uuid
@@ -243,12 +222,6 @@ func UpdateAttachment(ctx context.Context, atta *Attachment) error {
 	return err
 }
 
-// DeleteAttachmentsByRelease deletes all attachments associated with the given release.
-func DeleteAttachmentsByRelease(ctx context.Context, releaseID int64) error {
-	_, err := db.GetEngine(ctx).Where("release_id = ?", releaseID).Delete(&Attachment{})
-	return err
-}
-
 // CountOrphanedAttachments returns the number of bad attachments
 func CountOrphanedAttachments(ctx context.Context) (int64, error) {
 	return db.GetEngine(ctx).Where("(issue_id > 0 and issue_id not in (select id from issue)) or (release_id > 0 and release_id not in (select id from `release`))").

models/repo/attachment_test.go

@@ -42,26 +42,6 @@ func TestGetByCommentOrIssueID(t *testing.T) {
 	assert.Len(t, attachments, 2)
 }
 
-func TestDeleteAttachments(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-
-	count, err := repo_model.DeleteAttachmentsByIssue(db.DefaultContext, 4, false)
-	assert.NoError(t, err)
-	assert.Equal(t, 2, count)
-
-	count, err = repo_model.DeleteAttachmentsByComment(db.DefaultContext, 2, false)
-	assert.NoError(t, err)
-	assert.Equal(t, 2, count)
-
-	err = repo_model.DeleteAttachment(db.DefaultContext, &repo_model.Attachment{ID: 8}, false)
-	assert.NoError(t, err)
-
-	attachment, err := repo_model.GetAttachmentByUUID(db.DefaultContext, "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a18")
-	assert.Error(t, err)
-	assert.True(t, repo_model.IsErrAttachmentNotExist(err))
-	assert.Nil(t, attachment)
-}
-
 func TestGetAttachmentByID(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 

models/system/storage_cleanup.go

@@ -0,0 +1,42 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package system
+
+import (
+	"context"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/timeutil"
+)
+
+const (
+	PathFile = 1 // PathTypeFile represents a file
+	PathDir  = 2 // PathTypeDir represents a directory
+)
+
+// StoragePathDeletion represents a file or directory that is pending deletion.
+type StoragePathDeletion struct {
+	ID                     int64
+	StorageName            string             // storage name defines in storage module
+	PathType               int                // 1 for file, 2 for directory
+	RelativePath           string             `xorm:"TEXT"`
+	DeleteFailedCount      int                `xorm:"DEFAULT 0 NOT NULL"` // Number of times the deletion failed, used to prevent infinite loop
+	LastDeleteFailedReason string             `xorm:"TEXT"`               // Last reason the deletion failed, used to prevent infinite loop
+	LastDeleteFailedTime   timeutil.TimeStamp // Last time the deletion failed, used to prevent infinite loop
+	CreatedUnix            timeutil.TimeStamp `xorm:"INDEX created"`
+}
+
+func init() {
+	db.RegisterModel(new(StoragePathDeletion))
+}
+
+func UpdateDeletionFailure(ctx context.Context, deletion *StoragePathDeletion, err error) error {
+	deletion.DeleteFailedCount++
+	_, updateErr := db.GetEngine(ctx).Table("storage_path_deletion").ID(deletion.ID).Update(map[string]any{
+		"delete_failed_count":       deletion.DeleteFailedCount,
+		"last_delete_failed_reason": err.Error(),
+		"last_delete_failed_time":   timeutil.TimeStampNow(),
+	})
+	return updateErr
+}

models/user/main_test.go

@@ -7,6 +7,8 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/services/storagecleanup"
 
 	_ "code.gitea.io/gitea/models"
 	_ "code.gitea.io/gitea/models/actions"
@@ -15,5 +17,10 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	unittest.MainTest(m)
+	unittest.MainTest(m, &unittest.TestOptions{
+		SetUp: func() error {
+			setting.LoadQueueSettings()
+			return storagecleanup.Init()
+		},
+	})
 }

modules/storage/storage.go

@@ -166,6 +166,40 @@ func NewStorage(typStr Type, cfg *setting.Storage) (ObjectStorage, error) {
 	return fn(context.Background(), cfg)
 }
 
+const (
+	AttachmentStorageName       = "attachment"
+	AvatarStorageName           = "avatar"
+	RepoAvatarStorageName       = "repo_avatar"
+	LFSStorageName              = "lfs"
+	RepoArchiveStorageName      = "repo_archive"
+	PackagesStorageName         = "packages"
+	ActionsLogStorageName       = "actions_logs"
+	ActionsArtifactsStorageName = "actions_artifacts"
+)
+
+func GetStorageByName(name string) (ObjectStorage, error) {
+	switch name {
+	case AttachmentStorageName:
+		return Attachments, nil
+	case AvatarStorageName:
+		return Avatars, nil
+	case RepoAvatarStorageName:
+		return RepoAvatars, nil
+	case LFSStorageName:
+		return LFS, nil
+	case RepoArchiveStorageName:
+		return RepoArchives, nil
+	case PackagesStorageName:
+		return Packages, nil
+	case ActionsLogStorageName:
+		return Actions, nil
+	case ActionsArtifactsStorageName:
+		return ActionsArtifacts, nil
+	default:
+		return nil, fmt.Errorf("Unknown storage name: %s", name)
+	}
+}
+
 func initAvatars() (err error) {
 	log.Info("Initialising Avatar storage with type: %s", setting.Avatar.Storage.Type)
 	Avatars, err = NewStorage(setting.Avatar.Storage.Type, setting.Avatar.Storage)

options/locale/locale_en-US.ini

@@ -3065,6 +3065,7 @@ dashboard.sync_branch.started = Branches Sync started
 dashboard.sync_tag.started = Tags Sync started
 dashboard.rebuild_issue_indexer = Rebuild issue indexer
 dashboard.sync_repo_licenses = Sync repo licenses
+dashboard.cleanup_storage = Clean up deleted storage files
 
 users.user_manage_panel = User Account Management
 users.new_account = Create User Account

routers/api/v1/repo/issue_attachment.go

@@ -318,7 +318,7 @@ func DeleteIssueAttachment(ctx *context.APIContext) {
 		return
 	}
 
-	if err := repo_model.DeleteAttachment(ctx, attachment, true); err != nil {
+	if err := attachment_service.DeleteAttachment(ctx, attachment); err != nil {
 		ctx.APIErrorInternal(err)
 		return
 	}

routers/api/v1/repo/issue_comment_attachment.go

@@ -330,7 +330,7 @@ func DeleteIssueCommentAttachment(ctx *context.APIContext) {
 		return
 	}
 
-	if err := repo_model.DeleteAttachment(ctx, attach, true); err != nil {
+	if err := attachment_service.DeleteAttachment(ctx, attach); err != nil {
 		ctx.APIErrorInternal(err)
 		return
 	}

routers/api/v1/repo/release_attachment.go

@@ -393,8 +393,7 @@ func DeleteReleaseAttachment(ctx *context.APIContext) {
 		return
 	}
 	// FIXME Should prove the existence of the given repo, but results in unnecessary database requests
-
-	if err := repo_model.DeleteAttachment(ctx, attach, true); err != nil {
+	if err := attachment_service.DeleteAttachment(ctx, attach); err != nil {
 		ctx.APIErrorInternal(err)
 		return
 	}

routers/init.go

@@ -52,6 +52,7 @@ import (
 	release_service "code.gitea.io/gitea/services/release"
 	repo_service "code.gitea.io/gitea/services/repository"
 	"code.gitea.io/gitea/services/repository/archiver"
+	"code.gitea.io/gitea/services/storagecleanup"
 	"code.gitea.io/gitea/services/task"
 	"code.gitea.io/gitea/services/uinotification"
 	"code.gitea.io/gitea/services/webhook"
@@ -174,6 +175,7 @@ func InitWebInstalled(ctx context.Context) {
 	mustInitCtx(ctx, actions_service.Init)
 
 	mustInit(repo_service.InitLicenseClassifier)
+	mustInit(storagecleanup.Init)
 
 	// Finally start up the cron
 	cron.NewContext(ctx)