Improve attachments deletion #35103

lunny · 2025-07-16T20:31:45Z

Refactor attachment deletion logic

The attachment of comment deletion process has been moved from the AfterDelete hook to the service layer. This change avoids scenarios where files are deleted while the database transaction is later rolled back. The new implementation introduces a two-stage deletion process using a status column to mark attachments as deleted. These marked attachments are excluded from all UI and API queries. A background cleanup queue is responsible for permanently deleting the files and then removing the corresponding database records. If file deletion fails repeatedly, a system notice will be issued to alert administrators.

This will also improve performance when deleting a release with many files.

Removed unused functions

DeleteAttachmentsByIssue and DeleteAttachmentsByComment are removed because only tests need them.

wxiaoguang

Overall doesn't look good .....

wxiaoguang · 2025-07-20T14:48:31Z

models/db/file_status.go

+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package db


Why in db package? No proper package for it?

Because the enum can be reused for other storages in the future.

wxiaoguang · 2025-07-20T14:48:53Z

models/db/file_status.go

+type FileStatus int
+
+const (
+	FileStatusNormal      FileStatus = iota // FileStatusNormal indicates the file is normal and exists on disk.


Zero values cause various problems in XORM

wxiaoguang · 2025-07-20T14:50:14Z

models/issues/comment.go

+		if res > 0 {
+			var reviewComment Comment
+			has, err := db.GetEngine(ctx).Where("review_id = ?", comment.ReviewID).
+				And("type = ?", CommentTypeReview).Get(&reviewComment)


type, quote or not?

wxiaoguang · 2025-07-20T14:50:58Z

models/issues/comment.go

+	// delete review & review comment if the code comment is the last comment of the review
+	if comment.Type == CommentTypeCode && comment.ReviewID > 0 {
+		res, err := db.GetEngine(ctx).ID(comment.ReviewID).
+			Where("NOT EXISTS (SELECT 1 FROM comment WHERE review_id = ? AND `type` = ?)", comment.ReviewID, CommentTypeCode).


It needs to test with all databases since raw SQL is used.

Moved to #35133

wxiaoguang · 2025-07-20T14:51:52Z

models/issues/issue_update.go

@@ -305,6 +305,9 @@ 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 != 0 {


Will it cause problems if an attachment link is copied from issue-1 to issue-2?

The attachment was uploaded with no issue_id assigned when creating a new issue. And then invoking this method to update the issue id when submit the issue. The change will prevent one attachment which has been assigned an issue id to be changed to another.

If an attachment link copied to another place or outside of Gitea site. It will return 404 if the attachment is deleted. I don't understand what's your concern.

wxiaoguang · 2025-07-20T14:52:17Z

models/migrations/v1_25/v321.go

+		LastDeleteFailedTime timeutil.TimeStamp // Last time the deletion failed, used to prevent infinite loop
+		Size                 int64              `xorm:"DEFAULT 0"`
+		CreatedUnix          timeutil.TimeStamp `xorm:"created"`
+		CustomDownloadURL    string             `xorm:"-"`


Why a full copy-paste to the Attachment struct?

I remove this line. Keeping other columns there so that there is a snapshot of what the table should be at that time.

wxiaoguang · 2025-07-20T14:52:41Z

models/migrations/v1_25/v321.go

+		return err
+	}
+
+	if _, err := x.Exec("UPDATE `attachment` SET status = ? WHERE status IS NULL", db.FileStatusNormal); err != nil {


Why "null-able"?

wxiaoguang · 2025-07-20T14:56:48Z

services/attachment/attachment.go

+var cleanQueue *queue.WorkerPoolQueue[int64]
+
+func Init() error {
+	cleanQueue = queue.CreateSimpleQueue(graceful.GetManager().ShutdownContext(), "attachments-clean", handler)


Why it needs to use a queue?

A cleanup queue helps avoid immediate deletion of attachment files from disk, reducing blocking time for end users - especially when deleting large releases or repositories with many attachments. The associated cleanup cron task does not need to run frequently because of clean queue exist; cron task currently executes once every 24 hours. This cron task primarily handles rare edge cases, such as when a database transaction is successfully committed but the system restarts before the corresponding IDs are pushed to the queue. In most typical scenarios, the queue is processed shortly after the database transaction completes.

wxiaoguang · 2025-07-20T15:00:44Z

web_src/js/features/repo-issue.ts

+
+        if (json.deletedReviewCommentHashTag) {
+          document.querySelector(`#${json.deletedReviewCommentHashTag}`)?.remove();
+        }


#${json.deletedReviewCommentHashTag} should be the parent of the "delete button"? Either you should use "closest", or use a "data-xxxx" to make the delete button can find the target, but not use the fragile backend generated ID as selector.

moved to #35133

wxiaoguang · 2025-07-20T15:05:02Z

services/attachment/attachment.go

+	lastID := int64(0)
+	for {
+		if err := db.GetEngine(ctx).
+			Where("id > ? AND status = ?", lastID, db.FileStatusToBeDeleted).


It is very low slow. You need an index like this: (status, id)

wxiaoguang · 2025-07-20T15:05:45Z

services/attachment/attachment.go

+		if err := db.GetEngine(ctx).
+			Where("id > ? AND status = ?", lastID, db.FileStatusToBeDeleted).
+			Limit(100).
+			Find(&attachments); err != nil {


It generates wrong result since you do not make them have a stable order.

wxiaoguang · 2025-07-20T15:12:07Z

models/issues/comment.go

+
+	var deletedReviewComment *Comment
+
+	// delete review & review comment if the code comment is the last comment of the review


Why it can delete the review? What if the review is a "change request" or "approval"?

moved to #35133

wxiaoguang · 2025-07-20T15:13:14Z

services/issue/comments.go

 	}

+	attachment.AddAttachmentsToCleanQueue(ctx, comment.Attachments)


You deleted 2 comments above, but only handle one comment's "attachments"?

The review comment will be deleted if it contains no content or attachments, and its associated code comment is the last remaining one. If it has any attachments, it should not be deleted automatically.

The reviewComment deletion has been moved to #35133, now only one comment deleted in this pull request.

wxiaoguang · 2025-07-20T15:16:29Z

models/issues/comment.go

+
+	var deletedReviewComment *Comment
+
+	// delete review & review comment if the code comment is the last comment of the review


There is no test for such a changed behavior?

moved to #35133

wxiaoguang · 2025-07-20T15:17:52Z

Overall doesn't look good .....

Change my opinion: it looks wrong.

lunny · 2025-07-21T01:14:51Z

Overall doesn't look good .....

Change my opinion: it looks wrong.

I’ve pushed some new commits. Could you please review them again and let me know if there are any concerns?

wxiaoguang · 2025-07-21T01:33:22Z

Overall doesn't look good .....

Change my opinion: it looks wrong.

I’ve pushed some new commits. Could you please review them again and let me know if there are any concerns?

The design is wrong and not worth to review.

If your plan is "the enum can be reused for other storages in the future", you need a separate table like "storage_file_deletion (storage_type, storage_path)", but not keep patching other tables.

…torage could reuse the deletion infrastructure

lunny · 2025-07-21T05:57:38Z

@lafriks @wxiaoguang I reimplemented it and use a standalone table storage_path_deletion to handle file deletions. Please review again.

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 16, 2025

lunny added the type/bug label Jul 16, 2025

github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/frontend labels Jul 16, 2025

lunny added the backport/v1.24 This PR should be backported to Gitea 1.24 label Jul 16, 2025

github-actions bot added the modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin label Jul 17, 2025

lunny removed the backport/v1.24 This PR should be backported to Gitea 1.24 label Jul 17, 2025

lunny marked this pull request as draft July 17, 2025 07:04

lunny force-pushed the lunny/fix_bug_delete_code_comment branch from 026b762 to f3d0e51 Compare July 18, 2025 00:43

github-actions bot removed the modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin label Jul 18, 2025

lunny added 13 commits July 18, 2025 17:44

Fix deleting code comment bug

000d87e

improve the test

012b4e5

Remove AfterDelete function in comment

5cc3388

Fix bug

2d423dc

improvements

3abb757

improvements

8f97a2d

improvements

1742489

fix test

63173a6

revert unnecessary change

6b66c30

revert unnecessary change

cdb6147

Rename the function for post database transaction

85e6668

Rename

f868da0

Improve attachments deletions

97556a8

lunny force-pushed the lunny/fix_bug_delete_code_comment branch from 4ae54c8 to 97556a8 Compare July 19, 2025 00:44

github-actions bot added the modifies/migrations label Jul 19, 2025

lunny added 4 commits July 18, 2025 17:56

improvements

b89b277

improvements

488e658

improvements

e00c7d3

improvements

27c83de

lunny marked this pull request as ready for review July 19, 2025 21:33

lafriks approved these changes Jul 20, 2025

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 20, 2025

wxiaoguang requested changes Jul 20, 2025

View reviewed changes

GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 20, 2025

wxiaoguang reviewed Jul 20, 2025

View reviewed changes

lunny added 7 commits July 20, 2025 12:17

Fix some problems

9c251fd

revert unnecessary change in this pull request

0c9f37d

revert unnecessary change in this pull request

70d2960

revert unnecessary change in this pull request

f24dc5e

Fix bug

9faf99d

fix lint

df456dd

Revert unnecessary changes

fa5fc02

github-actions bot removed the modifies/frontend label Jul 20, 2025

lunny added 4 commits July 20, 2025 16:47

Fix test

7134ad9

Fix

e50adec

Merge branch 'main' into lunny/fix_bug_delete_code_comment

ccb8c9b

Fix test

8f8dd8c

lunny added 2 commits July 20, 2025 22:02

Use a standalone table to store deletion files so that all kinds of s…

3b2e424

…torage could reuse the deletion infrastructure

Some improvements

a159176


		var deletedReviewComment *Comment

		// delete review & review comment if the code comment is the last comment of the review

		}

		attachment.AddAttachmentsToCleanQueue(ctx, comment.Attachments)

Uh oh!

Improve attachments deletion #35103

Are you sure you want to change the base?

Improve attachments deletion #35103

Conversation

lunny commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refactor attachment deletion logic

Removed unused functions

Uh oh!

wxiaoguang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lunny Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang commented Jul 20, 2025

Uh oh!

lunny commented Jul 21, 2025

lunny commented Jul 16, 2025 •

edited

Loading

wxiaoguang Jul 20, 2025 •

edited

Loading

wxiaoguang Jul 20, 2025 •

edited

Loading

lunny Jul 20, 2025 •

edited

Loading