Improve attachments deletion #35103
Improve attachments deletion #35103
Conversation
GiteaBot
added
the
lgtm/need 2
github-actions
bot
added
modifies/api
github-actions
bot
added
the
modifies/cli
lunny
force-pushed
the
lunny/fix_bug_delete_code_comment
branch
from
026b762 to
f3d0e51
Compare
github-actions
bot
removed
the
modifies/cli
lunny
force-pushed
the
lunny/fix_bug_delete_code_comment
branch
from
4ae54c8 to
97556a8
Compare
GiteaBot
added
lgtm/need 1
models/issues/issue_update.go
Outdated
| @@ -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 { | |||
There was a problem hiding this comment.
Will it cause problems if an attachment link is copied from issue-1 to issue-2?
There was a problem hiding this comment.
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.
services/attachment/attachment.go
Outdated
| var cleanQueue *queue.WorkerPoolQueue[int64] | ||
|
|
||
| func Init() error { | ||
| cleanQueue = queue.CreateSimpleQueue(graceful.GetManager().ShutdownContext(), "attachments-clean", handler) |
There was a problem hiding this comment.
Why it needs to use a queue?
There was a problem hiding this comment.
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.
GiteaBot
added
lgtm/blocked
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
|
@lafriks @wxiaoguang I reimplemented it and use a standalone table |
Refactor attachment deletion logic
The attachment of comment deletion process has been moved from the
AfterDeletehook 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
DeleteAttachmentsByIssueandDeleteAttachmentsByCommentare removed because only tests need them.