From acba0b81398a59bee7b7269ce45fc7aab871e19c Mon Sep 17 00:00:00 2001 From: yushen Date: Tue, 31 Mar 2026 16:34:47 +0800 Subject: [PATCH] fix(upload): clean up S3 objects when attachments are deleted - Add Delete/DeleteKeys/KeyFromURL methods to S3Storage - DeleteAttachment handler now removes the S3 object after DB delete - DeleteComment collects attachment URLs before CASCADE, then cleans S3 - DeleteIssue collects all attachment URLs (issue + comment level) before CASCADE, then cleans S3 Co-Authored-By: Claude Opus 4.6 (1M context) --- server/internal/handler/comment.go | 5 +++ server/internal/handler/file.go | 22 ++++++++++ server/internal/handler/issue.go | 4 ++ server/internal/storage/s3.go | 40 ++++++++++++++++++ server/pkg/db/generated/attachment.sql.go | 51 +++++++++++++++++++++++ server/pkg/db/queries/attachment.sql | 9 ++++ 6 files changed, 131 insertions(+) diff --git a/server/internal/handler/comment.go b/server/internal/handler/comment.go index f05c07df..1d942d36 100644 --- a/server/internal/handler/comment.go +++ b/server/internal/handler/comment.go @@ -342,11 +342,16 @@ func (h *Handler) DeleteComment(w http.ResponseWriter, r *http.Request) { return } + // Collect attachment URLs before CASCADE delete removes them. + attachmentURLs, _ := h.Queries.ListAttachmentURLsByCommentID(r.Context(), parseUUID(commentId)) + if err := h.Queries.DeleteComment(r.Context(), parseUUID(commentId)); err != nil { slog.Warn("delete comment failed", append(logger.RequestAttrs(r), "error", err, "comment_id", commentId)...) writeError(w, http.StatusInternalServerError, "failed to delete comment") return } + + h.deleteS3Objects(r.Context(), attachmentURLs) slog.Info("comment deleted", append(logger.RequestAttrs(r), "comment_id", commentId, "issue_id", uuidToString(comment.IssueID))...) h.publish(protocol.EventCommentDeleted, workspaceID, actorType, actorID, map[string]any{ "comment_id": commentId, diff --git a/server/internal/handler/file.go b/server/internal/handler/file.go index 50bedc9d..85c71b48 100644 --- a/server/internal/handler/file.go +++ b/server/internal/handler/file.go @@ -1,6 +1,7 @@ package handler import ( + "context" "crypto/rand" "encoding/hex" "fmt" @@ -292,5 +293,26 @@ func (h *Handler) DeleteAttachment(w http.ResponseWriter, r *http.Request) { return } + h.deleteS3Object(r.Context(), att.Url) w.WriteHeader(http.StatusNoContent) } + +// deleteS3Object removes a single file from S3 by its CDN URL. +func (h *Handler) deleteS3Object(ctx context.Context, url string) { + if h.Storage == nil || url == "" { + return + } + h.Storage.Delete(ctx, h.Storage.KeyFromURL(url)) +} + +// deleteS3Objects removes multiple files from S3 by their CDN URLs. +func (h *Handler) deleteS3Objects(ctx context.Context, urls []string) { + if h.Storage == nil || len(urls) == 0 { + return + } + keys := make([]string, len(urls)) + for i, u := range urls { + keys[i] = h.Storage.KeyFromURL(u) + } + h.Storage.DeleteKeys(ctx, keys) +} diff --git a/server/internal/handler/issue.go b/server/internal/handler/issue.go index ca3d9bd8..0cf7dc17 100644 --- a/server/internal/handler/issue.go +++ b/server/internal/handler/issue.go @@ -548,12 +548,16 @@ func (h *Handler) DeleteIssue(w http.ResponseWriter, r *http.Request) { h.TaskService.CancelTasksForIssue(r.Context(), issue.ID) + // Collect all attachment URLs (issue-level + comment-level) before CASCADE delete. + attachmentURLs, _ := h.Queries.ListAttachmentURLsByIssueOrComments(r.Context(), issue.ID) + err := h.Queries.DeleteIssue(r.Context(), parseUUID(id)) if err != nil { writeError(w, http.StatusInternalServerError, "failed to delete issue") return } + h.deleteS3Objects(r.Context(), attachmentURLs) userID := requestUserID(r) actorType, actorID := h.resolveActor(r, userID, uuidToString(issue.WorkspaceID)) h.publish(protocol.EventIssueDeleted, uuidToString(issue.WorkspaceID), actorType, actorID, map[string]any{"issue_id": id}) diff --git a/server/internal/storage/s3.go b/server/internal/storage/s3.go index 86167c18..b04a9561 100644 --- a/server/internal/storage/s3.go +++ b/server/internal/storage/s3.go @@ -83,6 +83,46 @@ func sanitizeFilename(name string) string { return b.String() } +// KeyFromURL extracts the S3 object key from a CDN or bucket URL. +// e.g. "https://multica-static.copilothub.ai/abc123.png" → "abc123.png" +func (s *S3Storage) KeyFromURL(rawURL string) string { + // Strip the "https://domain/" prefix. + for _, prefix := range []string{ + "https://" + s.cdnDomain + "/", + "https://" + s.bucket + "/", + } { + if strings.HasPrefix(rawURL, prefix) { + return strings.TrimPrefix(rawURL, prefix) + } + } + // Fallback: take everything after the last "/". + if i := strings.LastIndex(rawURL, "/"); i >= 0 { + return rawURL[i+1:] + } + return rawURL +} + +// Delete removes an object from S3. Errors are logged but not fatal. +func (s *S3Storage) Delete(ctx context.Context, key string) { + if key == "" { + return + } + _, err := s.client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: aws.String(s.bucket), + Key: aws.String(key), + }) + if err != nil { + slog.Error("s3 DeleteObject failed", "key", key, "error", err) + } +} + +// DeleteKeys removes multiple objects from S3. Best-effort, errors are logged. +func (s *S3Storage) DeleteKeys(ctx context.Context, keys []string) { + for _, key := range keys { + s.Delete(ctx, key) + } +} + func (s *S3Storage) Upload(ctx context.Context, key string, data []byte, contentType string, filename string) (string, error) { safe := sanitizeFilename(filename) _, err := s.client.PutObject(ctx, &s3.PutObjectInput{ diff --git a/server/pkg/db/generated/attachment.sql.go b/server/pkg/db/generated/attachment.sql.go index 858365ad..4f970adc 100644 --- a/server/pkg/db/generated/attachment.sql.go +++ b/server/pkg/db/generated/attachment.sql.go @@ -101,6 +101,57 @@ func (q *Queries) GetAttachment(ctx context.Context, arg GetAttachmentParams) (A return i, err } +const listAttachmentURLsByCommentID = `-- name: ListAttachmentURLsByCommentID :many +SELECT url FROM attachment +WHERE comment_id = $1 +` + +func (q *Queries) ListAttachmentURLsByCommentID(ctx context.Context, commentID pgtype.UUID) ([]string, error) { + rows, err := q.db.Query(ctx, listAttachmentURLsByCommentID, commentID) + if err != nil { + return nil, err + } + defer rows.Close() + items := []string{} + for rows.Next() { + var url string + if err := rows.Scan(&url); err != nil { + return nil, err + } + items = append(items, url) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listAttachmentURLsByIssueOrComments = `-- name: ListAttachmentURLsByIssueOrComments :many +SELECT a.url FROM attachment a +WHERE a.issue_id = $1 + OR a.comment_id IN (SELECT c.id FROM comment c WHERE c.issue_id = $1) +` + +func (q *Queries) ListAttachmentURLsByIssueOrComments(ctx context.Context, issueID pgtype.UUID) ([]string, error) { + rows, err := q.db.Query(ctx, listAttachmentURLsByIssueOrComments, issueID) + if err != nil { + return nil, err + } + defer rows.Close() + items := []string{} + for rows.Next() { + var url string + if err := rows.Scan(&url); err != nil { + return nil, err + } + items = append(items, url) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listAttachmentsByComment = `-- name: ListAttachmentsByComment :many SELECT id, workspace_id, issue_id, comment_id, uploader_type, uploader_id, filename, url, content_type, size_bytes, created_at FROM attachment WHERE comment_id = $1 AND workspace_id = $2 diff --git a/server/pkg/db/queries/attachment.sql b/server/pkg/db/queries/attachment.sql index 6003ab88..c22e20ea 100644 --- a/server/pkg/db/queries/attachment.sql +++ b/server/pkg/db/queries/attachment.sql @@ -22,5 +22,14 @@ SELECT * FROM attachment WHERE comment_id = ANY($1::uuid[]) ORDER BY created_at ASC; +-- name: ListAttachmentURLsByIssueOrComments :many +SELECT a.url FROM attachment a +WHERE a.issue_id = $1 + OR a.comment_id IN (SELECT c.id FROM comment c WHERE c.issue_id = $1); + +-- name: ListAttachmentURLsByCommentID :many +SELECT url FROM attachment +WHERE comment_id = $1; + -- name: DeleteAttachment :exec DELETE FROM attachment WHERE id = $1 AND workspace_id = $2;