Revisit keepMaxUnreads (#6632)

* Revisit keepMaxUnreads
Again, follow-up of https://github.com/FreshRSS/FreshRSS/pull/5905
fix https://github.com/FreshRSS/FreshRSS/issues/6620

* Refactoring to address buggy cases

* Fix minor test

app/Controllers/feedController.php

@@ -110,11 +110,7 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 		$feed->_id($id);
 
 		// Ok, feed has been added in database. Now we have to refresh entries.
-		[, , $nb_new_articles] = self::actualizeFeeds($id, $url);
-		if ($nb_new_articles > 0) {
-			self::commitNewEntries();
-		}
-
+		self::actualizeFeedsAndCommit($id, $url);
 		return $feed;
 	}
 
@@ -385,7 +381,8 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 	}
 
 	/**
-	 * @return array{0:int,1:FreshRSS_Feed|null,2:int} Number of updated feeds, first feed or null, number of new articles
+	 * @return array{0:int,1:FreshRSS_Feed|null,2:int,3:array<FreshRSS_Feed>} Number of updated feeds, first feed or null, number of new articles,
+	 * 	list of feeds for which a cache refresh is needed
 	 * @throws FreshRSS_BadUrl_Exception
 	 */
 	public static function actualizeFeeds(?int $feed_id = null, ?string $feed_url = null, ?int $maxFeeds = null, ?SimplePie $simplePiePush = null): array {
@@ -435,8 +432,9 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 		$pubsubhubbubEnabledGeneral = FreshRSS_Context::systemConf()->pubsubhubbub_enabled;
 		$pshbMinAge = time() - (3600 * 24);  //TODO: Make a configuration.
 
-		$updated_feeds = 0;
-		$nb_new_articles = 0;
+		$nbUpdatedFeeds = 0;
+		$nbNewArticles = 0;
+		$feedsCacheToRefresh = [];
 
 		foreach ($feeds as $feed) {
 			$feed = Minz_ExtensionManager::callHook('feed_before_actualize', $feed);
@@ -598,9 +596,6 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 							// If the entry has changed, there is a good chance for the full content to have changed as well.
 							$entry->loadCompleteContent(true);
 
-							if (!$entryDAO->inTransaction()) {
-								$entryDAO->beginTransaction();
-							}
 							$entryDAO->updateEntry($entry->toArray());
 						}
 					} else {
@@ -619,6 +614,8 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 							$titlesAsRead[$entry->title()] = true;
 						}
 
+						$needFeedCacheRefresh = true;
+
 						if ($pubSubHubbubEnabled && !$simplePiePush) {	//We use push, but have discovered an article by pull!
 							$text = 'An article was discovered by pull although we use PubSubHubbub!: Feed ' .
 								SimplePie_Misc::url_remove_credentials($url) .
@@ -629,29 +626,20 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 							$feed->pubSubHubbubError(true);
 						}
 
-						if (!$entryDAO->inTransaction()) {
-							$entryDAO->beginTransaction();
+						if ($entryDAO->addEntry($entry->toArray(), true)) {
+							$nbNewArticles++;
 						}
-						$entryDAO->addEntry($entry->toArray(), true);
-
-						$nb_new_articles++;
 					}
 				}
 				// N.B.: Applies to _entry table and not _entrytmp:
 				$entryDAO->updateLastSeen($feed->id(), array_keys($newGuids), $mtime);
 			} elseif ($feedIsUnchanged) {
 				// Feed cache was unchanged, so mark as seen the same entries as last time
-				if (!$entryDAO->inTransaction()) {
-					$entryDAO->beginTransaction();
-				}
 				$entryDAO->updateLastSeenUnchanged($feed->id(), $mtime);
 			}
 			unset($entries);
 
 			if (rand(0, 30) === 1) {	// Remove old entries once in 30.
-				if (!$entryDAO->inTransaction()) {
-					$entryDAO->beginTransaction();
-				}
 				$nb = $feed->cleanOldEntries();
 				if ($nb > 0) {
 					$needFeedCacheRefresh = true;
@@ -659,20 +647,12 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 			}
 
 			$feedDAO->updateLastUpdate($feed->id(), false, $mtime);
-			if ($feed->keepMaxUnread() !== null && ($feed->nbNotRead() + $nbMarkedUnread > $feed->keepMaxUnread())) {
-				Minz_Log::debug('Existing unread entries (' . ($feed->nbNotRead() + $nbMarkedUnread) . ') exceeding max number of ' .
-					$feed->keepMaxUnread() .  ' for [' . $feed->url(false) . ']');
-				$needFeedCacheRefresh |= ($feed->markAsReadMaxUnread() != false);
-			}
 			if ($simplePiePush === null) {
 				// Do not call for WebSub events, as we do not know the list of articles still on the upstream feed.
 				$needFeedCacheRefresh |= ($feed->markAsReadUponGone($feedIsEmpty, $mtime) != false);
 			}
 			if ($needFeedCacheRefresh) {
-				$feedDAO->updateCachedValues($feed->id());
-			}
-			if ($entryDAO->inTransaction()) {
-				$entryDAO->commit();
+				$feedsCacheToRefresh[] = $feed;
 			}
 
 			$feedProperties = [];
@@ -735,43 +715,37 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 				}
 			}
 			$feed->unlock();
-			$updated_feeds++;
+			$nbUpdatedFeeds++;
 			unset($feed);
 			gc_collect_cycles();
 
-			if ($updated_feeds >= $maxFeeds) {
+			if ($nbUpdatedFeeds >= $maxFeeds) {
 				break;
 			}
 		}
-		return [$updated_feeds, reset($feeds) ?: null, $nb_new_articles];
+		return [$nbUpdatedFeeds, reset($feeds) ?: null, $nbNewArticles, $feedsCacheToRefresh];
 	}
 
 	/**
-	 * @return int|false The number of articles marked as read, of false if error
+	 * Feeds on which to apply a the keep max unreads policy, or all feeds if none specified.
+	 * @return int The number of articles marked as read
 	 */
-	private static function keepMaxUnreads() {
+	private static function keepMaxUnreads(FreshRSS_Feed ...$feeds): int {
 		$affected = 0;
-		$entryDAO = FreshRSS_Factory::createEntryDao();
-		$newUnreadEntriesPerFeed = $entryDAO->newUnreadEntriesPerFeed();
-		$feedDAO = FreshRSS_Factory::createFeedDao();
-		$feeds = $feedDAO->listFeedsOrderUpdate(-1);
+
+		if (empty($feeds)) {
+			$feedDAO = FreshRSS_Factory::createFeedDao();
+			$feeds = $feedDAO->listFeedsOrderUpdate(-1);
+		}
+
 		foreach ($feeds as $feed) {
-			if (!empty($newUnreadEntriesPerFeed[$feed->id()]) && $feed->keepMaxUnread() !== null &&
-				($feed->nbNotRead() + $newUnreadEntriesPerFeed[$feed->id()] > $feed->keepMaxUnread())) {
-				Minz_Log::debug('New unread entries (' . ($feed->nbNotRead() + $newUnreadEntriesPerFeed[$feed->id()]) . ') exceeding max number of ' .
-					$feed->keepMaxUnread() .  ' for [' . $feed->url(false) . ']');
-				$n = $feed->markAsReadMaxUnread();
-				if ($n === false) {
-					$affected = false;
-					break;
-				} else {
-					$affected += $n;
-				}
+			$n = $feed->markAsReadMaxUnread();
+			if ($n !== false && $n > 0) {
+				Minz_Log::debug($n . ' unread entries exceeding max number of ' . $feed->keepMaxUnread() .  ' for [' . $feed->url(false) . ']');
+				$affected += $n;
 			}
 		}
-		if ($feedDAO->updateCachedValues() === false) {
-			$affected = false;
-		}
+
 		return $affected;
 	}
 
@@ -807,23 +781,38 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 		return $tagDAO->tagEntries($applyLabels);
 	}
 
-	public static function commitNewEntries(): void {
+	public static function commitNewEntries(): int {
 		$entryDAO = FreshRSS_Factory::createEntryDao();
-		['all' => $nbNewEntries, 'unread' => $nbNewUnreadEntries] = $entryDAO->countNewEntries();
+		$nbNewEntries = $entryDAO->countNewEntries();
 		if ($nbNewEntries > 0) {
-			if (!$entryDAO->inTransaction()) {
-				$entryDAO->beginTransaction();
-			}
 			if ($entryDAO->commitNewEntries()) {
 				self::applyLabelActions($nbNewEntries);
-				if ($nbNewUnreadEntries > 0) {
-					self::keepMaxUnreads();
-				}
-			}
-			if ($entryDAO->inTransaction()) {
-				$entryDAO->commit();
 			}
 		}
+		return $nbNewEntries;
+	}
+
+	/**
+	 * @return array{0:int,1:FreshRSS_Feed|null,2:int,3:array<FreshRSS_Feed>} Number of updated feeds, first feed or null, number of new articles,
+	 * 	list of feeds for which a cache refresh is needed
+	 * @throws FreshRSS_BadUrl_Exception
+	 */
+	public static function actualizeFeedsAndCommit(?int $feed_id = null, ?string $feed_url = null, ?int $maxFeeds = null, ?SimplePie $simplePiePush = null): array {
+		$entryDAO = FreshRSS_Factory::createEntryDao();
+		$entryDAO->beginTransaction();
+		[$nbUpdatedFeeds, $feed, $nbNewArticles, $feedsCacheToRefresh] = FreshRSS_feed_Controller::actualizeFeeds($feed_id, $feed_url, $maxFeeds, $simplePiePush);
+		if ($nbNewArticles > 0) {
+			FreshRSS_feed_Controller::commitNewEntries();
+		}
+		if (count($feedsCacheToRefresh) > 0) {
+			$feedDAO = FreshRSS_Factory::createFeedDao();
+			self::keepMaxUnreads(...$feedsCacheToRefresh);
+			$feedDAO->updateCachedValues(...array_map(fn(FreshRSS_Feed $f) => $f->id(), $feedsCacheToRefresh));
+		}
+		if ($entryDAO->inTransaction()) {
+			$entryDAO->commit();
+		}
+		return [$nbUpdatedFeeds, $feed, $nbNewArticles, $feedsCacheToRefresh];
 	}
 
 	/**
@@ -844,9 +833,11 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 		$noCommit = ($_POST['noCommit'] ?? 0) == 1;
 
 		if ($id === -1 && !$noCommit) {	//Special request only to commit & refresh DB cache
-			$updated_feeds = 0;
+			$nbUpdatedFeeds = 0;
 			$feed = null;
-			self::commitNewEntries();
+			FreshRSS_feed_Controller::commitNewEntries();
+			$feedDAO = FreshRSS_Factory::createFeedDao();
+			$feedDAO->updateCachedValues();
 		} else {
 			if ($id === 0 && $url === '') {
 				// Case of a batch refresh (e.g. cron)
@@ -855,11 +846,32 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 				Minz_ExtensionManager::callHookVoid('freshrss_user_maintenance');
 
 				FreshRSS_feed_Controller::commitNewEntries();
+				$feedDAO = FreshRSS_Factory::createFeedDao();
+				$feedDAO->updateCachedValues();
 				FreshRSS_category_Controller::refreshDynamicOpmls();
 			}
-			[$updated_feeds, $feed, $nbNewArticles] = self::actualizeFeeds($id, $url, $maxFeeds);
-			if (!$noCommit && $nbNewArticles > 0) {
-				FreshRSS_feed_Controller::commitNewEntries();
+			$entryDAO = FreshRSS_Factory::createEntryDao();
+			$entryDAO->beginTransaction();
+			[$nbUpdatedFeeds, $feed, $nbNewArticles, $feedsCacheToRefresh] = self::actualizeFeeds($id, $url, $maxFeeds);
+			if (!$noCommit) {
+				if ($nbNewArticles > 0) {
+					FreshRSS_feed_Controller::commitNewEntries();
+				}
+				$feedDAO = FreshRSS_Factory::createFeedDao();
+				if ($id !== 0 && $id !== -1) {
+					if ($feed instanceof FreshRSS_Feed) {
+						self::keepMaxUnreads($feed);
+					}
+					// Case of single feed refreshed, always update its cache
+					$feedDAO->updateCachedValues($id);
+				} elseif (count($feedsCacheToRefresh) > 0) {
+					self::keepMaxUnreads(...$feedsCacheToRefresh);
+					// Case of multiple feeds refreshed, only update cache of affected feeds
+					$feedDAO->updateCachedValues(...array_map(fn(FreshRSS_Feed $f) => $f->id(), $feedsCacheToRefresh));
+				}
+			}
+			if ($entryDAO->inTransaction()) {
+				$entryDAO->commit();
 			}
 		}
 
@@ -875,12 +887,12 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 			Minz_Request::good(_t('feedback.sub.feed.actualized', $feed->name()), [
 				'params' => ['get' => 'f_' . $id]
 			]);
-		} elseif ($updated_feeds >= 1) {
-			Minz_Request::good(_t('feedback.sub.feed.n_actualized', $updated_feeds), []);
+		} elseif ($nbUpdatedFeeds >= 1) {
+			Minz_Request::good(_t('feedback.sub.feed.n_actualized', $nbUpdatedFeeds), []);
 		} else {
 			Minz_Request::good(_t('feedback.sub.feed.no_refresh'), []);
 		}
-		return $updated_feeds;
+		return $nbUpdatedFeeds;
 	}
 
 	/**
@@ -1047,8 +1059,6 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 		$limit = Minz_Request::paramInt('reload_limit') ?: 10;
 
 		$feedDAO = FreshRSS_Factory::createFeedDao();
-		$entryDAO = FreshRSS_Factory::createEntryDao();
-
 		$feed = $feedDAO->searchById($feed_id);
 		if ($feed === null) {
 			Minz_Request::bad(_t('feedback.sub.feed.not_found'), []);
@@ -1057,12 +1067,10 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 
 		//Re-fetch articles as if the feed was new.
 		$feedDAO->updateFeed($feed->id(), [ 'lastUpdate' => 0 ]);
-		[, , $nb_new_articles] = self::actualizeFeeds($feed_id);
-		if ($nb_new_articles > 0) {
-			FreshRSS_feed_Controller::commitNewEntries();
-		}
+		self::actualizeFeedsAndCommit($feed_id);
 
 		//Extract all feed entries from database, load complete content and store them back in database.
+		$entryDAO = FreshRSS_Factory::createEntryDao();
 		$entries = $entryDAO->listWhere('f', $feed_id, FreshRSS_Entry::STATE_ALL, 'DESC', $limit);
 
 		//We need another DB connection in parallel for unbuffered streaming

app/Models/EntryDAO.php

@@ -274,45 +274,14 @@ SQL;
 	}
 
 	/**
-	 * Count the number of new entries in the temporary table (which have not yet been committed), grouped by read / unread.
-	 * @return array{'all':int,'unread':int,'read':int}
+	 * Count the number of new entries in the temporary table (which have not yet been committed).
 	 */
-	public function countNewEntries(): array {
+	public function countNewEntries(): int {
 		$sql = <<<'SQL'
-		SELECT is_read, COUNT(id) AS nb_entries FROM `_entrytmp`
-		GROUP BY is_read
+		SELECT COUNT(id) AS nb_entries FROM `_entrytmp`
 		SQL;
-		$lines = $this->fetchAssoc($sql) ?? [];
-		$nbRead = 0;
-		$nbUnread = 0;
-		foreach ($lines as $line) {
-			if (empty($line['is_read'])) {
-				$nbUnread = (int)($line['nb_entries'] ?? 0);
-			} else {
-				$nbRead = (int)($line['nb_entries'] ?? 0);
-			}
-		}
-		return ['all' => $nbRead + $nbUnread, 'unread' => $nbUnread, 'read' => $nbRead];
-	}
-
-	/**
-	 * Count the number of new unread entries in the temporary table (which have not yet been committed), grouped by feed ID.
-	 * @return array<int,int>
-	 */
-	public function newUnreadEntriesPerFeed(): array {
-		$sql = <<<'SQL'
-		SELECT id_feed, COUNT(id) AS nb_entries FROM `_entrytmp`
-		WHERE is_read = 0
-		GROUP BY id_feed
-		SQL;
-		$lines = $this->fetchAssoc($sql) ?? [];
-		$result = [];
-		foreach ($lines as $line) {
-			if (!empty($line['id_feed'])) {
-				$result[(int)$line['id_feed']] = (int)($line['nb_entries'] ?? 0);
-			}
-		}
-		return $result;
+		$res = $this->fetchColumn($sql, 0);
+		return isset($res[0]) ? (int)$res[0] : -1;
 	}
 
 	/**
@@ -651,7 +620,7 @@ SQL;
 	}
 
 	/**
-	 * Remember to call updateCachedValue($id_feed) or updateCachedValues() just after.
+	 * Remember to call updateCachedValues($id_feed) or updateCachedValues() just after.
 	 * @param array<string,bool|int|string> $options
 	 * @return int|false
 	 */

app/Models/Feed.php

@@ -835,15 +835,12 @@ class FreshRSS_Feed extends Minz_Model {
 		}
 		$feedDAO = FreshRSS_Factory::createFeedDao();
 		$affected = $feedDAO->markAsReadMaxUnread($this->id(), $keepMaxUnread);
-		if ($affected > 0) {
-			Minz_Log::debug(__METHOD__ . " $affected items [" . $this->url(false) . ']');
-		}
 		return $affected;
 	}
 
 	/**
 	 * Applies the *mark as read upon gone* policy, if enabled.
-	 * Remember to call `updateCachedValue($id_feed)` or `updateCachedValues()` just after.
+	 * Remember to call `updateCachedValues($id_feed)` or `updateCachedValues()` just after.
 	 * @return int|false the number of lines affected, or false if not applicable
 	 */
 	public function markAsReadUponGone(bool $upstreamIsEmpty, int $maxTimestamp = 0) {
@@ -871,7 +868,7 @@ class FreshRSS_Feed extends Minz_Model {
 	}
 
 	/**
-	 * Remember to call `updateCachedValue($id_feed)` or `updateCachedValues()` just after
+	 * Remember to call `updateCachedValues($id_feed)` or `updateCachedValues()` just after
 	 * @return int|false
 	 */
 	public function cleanOldEntries() {

app/Models/FeedDAO.php

@@ -203,7 +203,7 @@ class FreshRSS_FeedDAO extends Minz_ModelPdo {
 
 	/**
 	 * @return int|false
-	 * @see updateCachedValue()
+	 * @see updateCachedValues()
 	 */
 	public function updateLastUpdate(int $id, bool $inError = false, int $mtime = 0) {
 		$sql = 'UPDATE `_feed` SET `lastUpdate`=?, error=? WHERE id=?';
@@ -453,20 +453,21 @@ SQL;
 	}
 
 	/**
+	 * Update cached values for selected feeds, or all feeds if no feed ID is provided.
 	 * @return int|false
 	 */
-	public function updateCachedValues(int $id = 0) {
+	public function updateCachedValues(int ...$feedIds) {
 		//2 sub-requests with FOREIGN KEY(e.id_feed), INDEX(e.is_read) faster than 1 request with GROUP BY or CASE
-		$sql = 'UPDATE `_feed` '
-			. 'SET `cache_nbEntries`=(SELECT COUNT(e1.id) FROM `_entry` e1 WHERE e1.id_feed=`_feed`.id),'
-			. '`cache_nbUnreads`=(SELECT COUNT(e2.id) FROM `_entry` e2 WHERE e2.id_feed=`_feed`.id AND e2.is_read=0)'
-			. ($id != 0 ? ' WHERE id=:id' : '');
-		$stm = $this->pdo->prepare($sql);
-		if ($stm !== false && $id != 0) {
-			$stm->bindParam(':id', $id, PDO::PARAM_INT);
+		$sql = <<<SQL
+UPDATE `_feed`
+SET `cache_nbEntries`=(SELECT COUNT(e1.id) FROM `_entry` e1 WHERE e1.id_feed=`_feed`.id),
+	`cache_nbUnreads`=(SELECT COUNT(e2.id) FROM `_entry` e2 WHERE e2.id_feed=`_feed`.id AND e2.is_read=0)
+SQL;
+		if (count($feedIds) > 0) {
+			$sql .= ' WHERE id IN (' . str_repeat('?,', count($feedIds) - 1). '?)';
 		}
-
-		if ($stm !== false && $stm->execute()) {
+		$stm = $this->pdo->prepare($sql);
+		if ($stm !== false && $stm->execute($feedIds)) {
 			return $stm->rowCount();
 		} else {
 			$info = $stm == null ? $this->pdo->errorInfo() : $stm->errorInfo();

cli/actualize-user.php

@@ -29,6 +29,9 @@ $databaseDAO->minorDbMaintenance();
 Minz_ExtensionManager::callHookVoid('freshrss_user_maintenance');
 
 FreshRSS_feed_Controller::commitNewEntries();
+$feedDAO = FreshRSS_Factory::createFeedDao();
+$feedDAO->updateCachedValues();
+
 $result = FreshRSS_category_Controller::refreshDynamicOpmls();
 if (!empty($result['errors'])) {
 	$errors = $result['errors'];
@@ -39,10 +42,7 @@ if (!empty($result['successes'])) {
 	echo "FreshRSS refreshed $successes dynamic OPMLs for $username\n";
 }
 
-[$nbUpdatedFeeds, , $nbNewArticles] = FreshRSS_feed_Controller::actualizeFeeds();
-if ($nbNewArticles > 0) {
-	FreshRSS_feed_Controller::commitNewEntries();
-}
+[$nbUpdatedFeeds, , $nbNewArticles] = FreshRSS_feed_Controller::actualizeFeedsAndCommit();
 
 echo "FreshRSS actualized $nbUpdatedFeeds feeds for $username ($nbNewArticles new articles)\n";
 

p/api/greader.php

@@ -331,10 +331,7 @@ final class GReaderAPI {
 		$importService = new FreshRSS_Import_Service($user);
 		$importService->importOpml($opml);
 		if ($importService->lastStatus()) {
-			[, , $nb_new_articles] = FreshRSS_feed_Controller::actualizeFeeds();
-			if ($nb_new_articles > 0) {
-				FreshRSS_feed_Controller::commitNewEntries();
-			}
+			FreshRSS_feed_Controller::actualizeFeedsAndCommit();
 			invalidateHttpCache($user);
 			exit('OK');
 		} else {

p/api/pshb.php

@@ -133,11 +133,8 @@ foreach ($users as $userFilename) {
 		Minz_ExtensionManager::enableByList(FreshRSS_Context::userConf()->extensions_enabled, 'user');
 		Minz_Translate::reset(FreshRSS_Context::userConf()->language);
 
-		[$updated_feeds, , $nb_new_articles] = FreshRSS_feed_Controller::actualizeFeeds(null, $self, null, $simplePie);
-		if ($nb_new_articles > 0) {
-			FreshRSS_feed_Controller::commitNewEntries();
-		}
-		if ($updated_feeds > 0) {
+		[$nbUpdatedFeeds, ] = FreshRSS_feed_Controller::actualizeFeedsAndCommit(null, $self, null, $simplePie);
+		if ($nbUpdatedFeeds > 0) {
 			$nb++;
 		} else {
 			Minz_Log::warning('Warning: User ' . $username . ' does not subscribe anymore to ' . $self, PSHB_LOG);