change DeleteOldItems logic

src/storage/item.go

@@ -249,7 +249,7 @@ func (s *Storage) CountItems(filter ItemFilter) int {
 	var count int
 	query := fmt.Sprintf(`
 		select count(*)
-		from items
+		from items i
 		where %s
 		`, predicate)
 	err := s.db.QueryRow(query, args...).Scan(&count)
@@ -435,67 +435,35 @@ var (
 //
 // The rules:
 //   - Never delete starred entries.
-//   - Keep at least the same amount of articles the feed provides (default: 50).
-//     This prevents from deleting items for rarely updated and/or ever-growing
-//     feeds which might eventually reappear as unread.
-//   - Keep entries for a certain period (default: 90 days).
+//   - Keep at least 50 latest items for each feed.
+//   - Delete entries older than 90 days relative to the latest arrived item in the same feed.
 func (s *Storage) DeleteOldItems() {
-	rows, err := s.db.Query(`
-		select
-			i.feed_id,
-			max(coalesce(s.size, 0), :keep_size) as max_items,
-			count(*) as num_items
-		from items i
-		left outer join feed_sizes s on s.feed_id = i.feed_id
-		where status != :starred_status
-		group by i.feed_id
-	`,
-		sql.Named("keep_size", itemsKeepSize),
-		sql.Named("starred_status", STARRED),
-	)
-	if err != nil {
-		log.Print(err)
-		return
-	}
-
-	feedLimits := make(map[int64]int64, 0)
-	for rows.Next() {
-		var feedId, limit int64
-		rows.Scan(&feedId, &limit, nil)
-		feedLimits[feedId] = limit
-	}
-
-	for feedId, limit := range feedLimits {
-		result, err := s.db.Exec(
-			`
+	result, err := s.db.Exec(`
 		delete from items
 		where id in (
-				select i.id
-				from items i
-				where i.feed_id = :feed_id and status != :starred_status
-				order by date desc
-				limit -1 offset :limit
-			) and date_arrived < :date_limit
-			`,
-			sql.Named("feed_id", feedId),
+			select id
+			from (
+				select
+					id,
+					row_number() over (partition by feed_id order by date desc) as rn,
+					last_arrived,
+					max(last_arrived) over (partition by feed_id) as max_la
+				from items
+				where status != :starred_status
+			)
+			where rn > :keep_size
+			  and last_arrived < datetime(max_la, :keep_days_limit)
+		)`,
 		sql.Named("starred_status", STARRED),
-			sql.Named("limit", limit),
-			sql.Named(
-				"date_limit",
-				time.Now().UTC().Add(-time.Hour*time.Duration(24*itemsKeepDays)),
-			),
+		sql.Named("keep_size", itemsKeepSize),
+		sql.Named("keep_days_limit", fmt.Sprintf("-%d days", itemsKeepDays)),
 	)
 	if err != nil {
 		log.Print(err)
 		return
 	}
 	numDeleted, err := result.RowsAffected()
-		if err != nil {
-			log.Print(err)
-			return
-		}
-		if numDeleted > 0 {
-			log.Printf("Deleted %d old items (feed: %d)", numDeleted, feedId)
-		}
+	if err == nil && numDeleted > 0 {
+		log.Printf("Deleted %d old items", numDeleted)
 	}
 }

src/storage/item_test.go

@@ -321,61 +321,76 @@ func TestMarkItemsRead(t *testing.T) {
 }
 
 func TestDeleteOldItems(t *testing.T) {
-	extraItems := 10
-
 	now := time.Now().UTC()
-	db := testDB()
-	feed := db.CreateFeed("feed", "", "", "http://test.com/feed11.xml", nil)
+	starred := STARRED
 
-	items := make([]Item, 0)
-	for i := 0; i < itemsKeepSize+extraItems; i++ {
-		istr := strconv.Itoa(i)
-		items = append(items, Item{
-			GUID:   istr,
-			FeedId: feed.Id,
-			Title:  istr,
-			Date:   now.Add(time.Hour * time.Duration(i)),
-		})
+	t.Run("keeps at least 50 items", func(t *testing.T) {
+		db := testDB()
+		feed := db.CreateFeed("f", "", "", "http://f.xml", nil)
+		items := make([]Item, 100)
+		for i := range 100 {
+			items[i] = Item{GUID: strconv.Itoa(i), FeedId: feed.Id, Date: now.Add(time.Duration(i) * time.Hour * 24)}
 		}
 		db.CreateItems(items)
 
-	db.SetFeedSize(feed.Id, itemsKeepSize)
-	var feedSize int
-	err := db.db.QueryRow(
-		`select size from feed_sizes where feed_id = :feed_id`, sql.Named("feed_id", feed.Id),
-	).Scan(&feedSize)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if feedSize != itemsKeepSize {
-		t.Fatalf(
-			"expected feed size to get updated\nwant: %d\nhave: %d",
-			itemsKeepSize+extraItems,
-			feedSize,
-		)
-	}
-
-	// expire only the first 3 articles
-	_, err = db.db.Exec(
-		`update items set date_arrived = :date_arrived
-		where id in (select id from items limit 3)`,
-		sql.Named("date_arrived", now.Add(-time.Hour*time.Duration(itemsKeepDays*24))),
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
+		// // Set 1 recent (latest), 100 old (100 days ago)
+		db.db.Exec(`update items set last_arrived = :la where guid = "99"`, sql.Named("la", now))
+		db.db.Exec(`update items set last_arrived = :la where guid != "99"`, sql.Named("la", now.Add(-time.Hour*24*100)))
 
 		db.DeleteOldItems()
-	feedItems := db.ListItems(ItemFilter{FeedID: &feed.Id}, 1000, false, false)
-	if len(feedItems) != len(items)-3 {
-		t.Fatalf(
-			"invalid number of old items kept\nwant: %d\nhave: %d",
-			len(items)-3,
-			len(feedItems),
-		)
+		have := db.CountItems(ItemFilter{FeedID: &feed.Id})
+		if have != 50 {
+			t.Errorf("expected 50 items, have %d", have)
 		}
+	})
+
+	t.Run("keeps all less than 90 days old", func(t *testing.T) {
+		db := testDB()
+		feed := db.CreateFeed("f", "", "", "http://f.xml", nil)
+		items := make([]Item, 100)
+		for i := 0; i < 100; i++ {
+			items[i] = Item{GUID: strconv.Itoa(i), FeedId: feed.Id, Date: now.Add(time.Duration(i) * time.Second)}
+		}
+		db.CreateItems(items)
+
+		// Latest item at "now"
+		// All others at 80 days ago (keep)
+		db.db.Exec(`update items set last_arrived = :la where guid = "99"`, sql.Named("la", now))
+		db.db.Exec(`update items set last_arrived = :la where guid != "99"`, sql.Named("la", now.Add(-time.Hour*24*80)))
+
+		db.DeleteOldItems()
+		have := db.CountItems(ItemFilter{FeedID: &feed.Id})
+		if have != 100 {
+			t.Errorf("expected 100 items, have %d", have)
+		}
+	})
+
+	t.Run("keeps starred", func(t *testing.T) {
+		db := testDB()
+		feed := db.CreateFeed("f", "", "", "http://f.xml", nil)
+		items := make([]Item, 100)
+		for i := 0; i < 100; i++ {
+			items[i] = Item{GUID: strconv.Itoa(i), FeedId: feed.Id, Date: now.Add(time.Duration(i) * time.Second)}
+		}
+		db.CreateItems(items)
+
+		// Set all to 100 days ago, except one recent
+		db.db.Exec(`update items set last_arrived = :la`, sql.Named("la", now.Add(-time.Hour*24*100)))
+		db.db.Exec(`update items set last_arrived = :la where guid = "99"`, sql.Named("la", now))
+		// Star 10 old items that would otherwise be deleted (rn > 50 and old)
+		db.db.Exec(`update items set status = :s where cast(guid as integer) < 10`, sql.Named("s", starred))
+
+		db.DeleteOldItems()
+		have := db.CountItems(ItemFilter{FeedID: &feed.Id})
+		// 50 (limit) + 10 (starred) = 60 items should remain.
+		if have != 60 {
+			t.Errorf("expected 60 items, have %d", have)
+		}
+	})
 }
 
+
+
 func TestCreateItemsLastArrived(t *testing.T) {
 	synctest.Test(t, func(t *testing.T) {
 		db := testDB()