4
0
Эх сурвалжийг харах

fix: avoid possible deadlock when cleaning removed entries

- Lock delete targets in order with SKIP LOCKED before deleting removed entries
- Have the removed-entry scrubber skip locked rows to prevent blocking
Frédéric Guillot 2 сар өмнө
parent
commit
2fa8995a35

+ 1 - 1
internal/cli/cleanup_tasks.go

@@ -47,7 +47,7 @@ func runCleanupTasks(store *storage.Storage) {
 		}
 	}
 
-	if enclosuresAffected, err := store.DeleteRemovedEntriesEnclosures(); err != nil {
+	if enclosuresAffected, err := store.DeleteEnclosuresOfRemovedEntries(); err != nil {
 		slog.Error("Unable to delete enclosures from removed entries", slog.Any("error", err))
 	} else {
 		slog.Info("Deleting enclosures from removed entries completed",

+ 21 - 0
internal/storage/enclosure.go

@@ -245,3 +245,24 @@ func (s *Storage) UpdateEnclosure(enclosure *model.Enclosure) error {
 
 	return nil
 }
+
+// DeleteEnclosuresOfRemovedEntries deletes enclosures associated with entries marked as "removed".
+func (s *Storage) DeleteEnclosuresOfRemovedEntries() (int64, error) {
+	query := `
+		DELETE FROM
+			enclosures
+		WHERE
+			enclosures.entry_id IN (SELECT id FROM entries WHERE status=$1)
+	`
+	result, err := s.db.Exec(query, model.EntryStatusRemoved)
+	if err != nil {
+		return 0, fmt.Errorf(`store: unable to delete enclosures from removed entries: %v`, err)
+	}
+
+	count, err := result.RowsAffected()
+	if err != nil {
+		return 0, fmt.Errorf(`store: unable to get the number of rows affected while deleting enclosures from removed entries: %v`, err)
+	}
+
+	return count, nil
+}

+ 19 - 28
internal/storage/entry.go

@@ -315,44 +315,34 @@ func (s *Storage) GetReadTime(feedID int64, entryHash string) int {
 
 // cleanupRemovedEntriesNotInFeed deletes from the database entries marked as "removed" and not visible anymore in the feed.
 func (s *Storage) cleanupRemovedEntriesNotInFeed(feedID int64, entryHashes []string) error {
+	// Acquire locks in id order and skip already-locked rows to avoid deadlocks with
+	// ClearRemovedEntriesContent, which also updates removed entries concurrently.
 	query := `
-		DELETE FROM
-			entries
-		WHERE
-			feed_id=$1 AND
-			status=$2 AND
-			NOT (hash=ANY($3))
+		WITH to_delete AS (
+			SELECT id
+			FROM entries
+			WHERE
+				feed_id=$1 AND
+				status=$2 AND
+				NOT (hash=ANY($3))
+			ORDER BY id
+			FOR UPDATE SKIP LOCKED
+		)
+		DELETE FROM entries
+		USING to_delete
+		WHERE entries.id = to_delete.id
 	`
 	if _, err := s.db.Exec(query, feedID, model.EntryStatusRemoved, pq.Array(entryHashes)); err != nil {
-		return fmt.Errorf(`store: unable to cleanup entries: %v`, err)
+		return fmt.Errorf(`store: unable to remove entries not in feed: %v`, err)
 	}
 
 	return nil
 }
 
-// DeleteRemovedEntriesEnclosures deletes enclosures associated with entries marked as "removed".
-func (s *Storage) DeleteRemovedEntriesEnclosures() (int64, error) {
-	query := `
-		DELETE FROM
-			enclosures
-		WHERE
-		 	enclosures.entry_id IN (SELECT id FROM entries WHERE status=$1)
-	`
-	result, err := s.db.Exec(query, model.EntryStatusRemoved)
-	if err != nil {
-		return 0, fmt.Errorf(`store: unable to delete enclosures from removed entries: %v`, err)
-	}
-
-	count, err := result.RowsAffected()
-	if err != nil {
-		return 0, fmt.Errorf(`store: unable to get the number of rows affected while deleting enclosures from removed entries: %v`, err)
-	}
-
-	return count, nil
-}
-
 // ClearRemovedEntriesContent clears the content fields of entries marked as "removed", keeping only their metadata.
 func (s *Storage) ClearRemovedEntriesContent(limit int) (int64, error) {
+	// Skip locked rows so this batch scrubber doesn't block or deadlock with the
+	// concurrent cleanup that deletes removed entries in the same table.
 	query := `
 		UPDATE
 			entries
@@ -368,6 +358,7 @@ func (s *Storage) ClearRemovedEntriesContent(limit int) (int64, error) {
 			FROM entries
 			WHERE status = $1 AND content IS NOT NULL
 			ORDER BY id ASC
+			FOR UPDATE SKIP LOCKED
 			LIMIT $2
 		)
 	`