Refactor lastSeen and markReadAsGone (#5470)

* Refactor lastSeen and markReadAsGone
Make the logic a bit more robust and explicit

* Remove forgotten SQL param

* Add test inTransaction

* More robust transaction

* Add a debug log

* Add max timestamp to markAsReadUponGone

* Reduce number of debug lines

* typing

* Better detection of when feed is empty

* More explicit case for push

app/Controllers/feedController.php

@@ -368,30 +368,27 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 			$url = $feed->url();	//For detection of HTTP 301
 
 			$pubSubHubbubEnabled = $pubsubhubbubEnabledGeneral && $feed->pubSubHubbubEnabled();
-			if ((!$simplePiePush) && (!$feed_id) && $pubSubHubbubEnabled && ($feed->lastUpdate() > $pshbMinAge)) {
+			if ($simplePiePush === null && $feed_id === 0 && $pubSubHubbubEnabled && ($feed->lastUpdate() > $pshbMinAge)) {
 				//$text = 'Skip pull of feed using PubSubHubbub: ' . $url;
 				//Minz_Log::debug($text);
 				//Minz_Log::debug($text, PSHB_LOG);
 				continue;	//When PubSubHubbub is used, do not pull refresh so often
 			}
 
-			$mtime = 0;
 			if ($feed->mute()) {
 				continue;	//Feed refresh is disabled
 			}
+			$mtime = $feed->cacheModifiedTime() ?: 0;
 			$ttl = $feed->ttl();
-			if ((!$simplePiePush) && (!$feed_id) &&
-				($feed->lastUpdate() + 10 >= time() - (
-					$ttl === FreshRSS_Feed::TTL_DEFAULT ? FreshRSS_Context::$user_conf->ttl_default : $ttl))) {
+			if ($ttl === FreshRSS_Feed::TTL_DEFAULT) {
+				$ttl = FreshRSS_Context::$user_conf->ttl_default;
+			}
+			if ($simplePiePush === null && $feed_id === 0 && (time() <= $feed->lastUpdate() + $ttl)) {
 				//Too early to refresh from source, but check whether the feed was updated by another user
-				$mtime = $feed->cacheModifiedTime() ?: 0;
-				if ($feed->lastUpdate() + 10 >= $mtime) {
+				if ($mtime <= 0 || $feed->lastUpdate() >= $mtime) {
 					continue;	//Nothing newer from other users
 				}
-				//Minz_Log::debug($feed->url(false) . ' was updated at ' . date('c', $mtime) . ' by another user');
-				//Will take advantage of the newer cache
-			} else {
-				$mtime = time();
+				Minz_Log::debug('Feed ' . $feed->url(false) . ' was updated at ' . date('c', $mtime) . ' by another user; will take advantage of the newer cache.');
 			}
 
 			if (!$feed->lock()) {
@@ -399,11 +396,12 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 				continue;
 			}
 
-			$isNewFeed = $feed->lastUpdate() <= 0;
+			$feedIsNew = $feed->lastUpdate() <= 0;
+			$feedIsEmpty = false;
 			$feedIsUnchanged = false;
 
 			try {
-				if ($simplePiePush) {
+				if ($simplePiePush !== null) {
 					$simplePie = $simplePiePush;	//Used by WebSub
 				} elseif ($feed->kind() === FreshRSS_Feed::KIND_HTML_XPATH) {
 					$simplePie = $feed->loadHtmlXpath();
@@ -416,14 +414,22 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 						throw new FreshRSS_Feed_Exception('XML+XPath parsing failed for [' . $feed->url(false) . ']');
 					}
 				} else {
-					$simplePie = $feed->load(false, $isNewFeed);
-					if ($simplePie === null) {
-						// Feed is cached and unchanged
-						$feedIsUnchanged = true;
-					}
+					$simplePie = $feed->load(false, $feedIsNew);
+				}
+
+				if ($simplePie === null) {
+					// Feed is cached and unchanged
+					$newGuids = [];
+					$entries = [];
+					$feedIsEmpty = false;	// We do not know
+					$feedIsUnchanged = true;
+				} else {
+					$newGuids = $feed->loadGuids($simplePie);
+					$entries = $feed->loadEntries($simplePie);
+					$feedIsEmpty = $simplePiePush !== null && empty($newGuids);
+					$feedIsUnchanged = false;
 				}
-				$newGuids = $simplePie === null ? [] : $feed->loadGuids($simplePie);
-				$entries = $simplePie === null ? [] : $feed->loadEntries($simplePie);
+				$mtime = $feed->cacheModifiedTime() ?: time();
 			} catch (FreshRSS_Feed_Exception $e) {
 				Minz_Log::warning($e->getMessage());
 				$feedDAO->updateLastUpdate($feed->id(), true);
@@ -553,9 +559,9 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 
 			$feedDAO->updateLastUpdate($feed->id(), false, $mtime);
 			$needFeedCacheRefresh |= ($feed->keepMaxUnread() != false);
-			if (!$simplePiePush) {
+			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() != false);
+				$needFeedCacheRefresh |= ($feed->markAsReadUponGone($feedIsEmpty, $mtime) != false);
 			}
 			if ($needFeedCacheRefresh) {
 				$feedDAO->updateCachedValues($feed->id());
@@ -608,7 +614,7 @@ class FreshRSS_feed_Controller extends FreshRSS_ActionController {
 			}
 			if (!empty($feedProperties)) {
 				$ok = $feedDAO->updateFeed($feed->id(), $feedProperties);
-				if (!$ok && $isNewFeed) {
+				if (!$ok && $feedIsNew) {
 					//Cancel adding new feed in case of database error at first actualize
 					$feedDAO->deleteFeed($feed->id());
 					$feed->unlock();

app/Models/EntryDAO.php

@@ -120,7 +120,7 @@ SQL;
 	 */
 	private $addEntryPrepared = false;
 
-	/** @param array{'id':string,'guid':string,'title':string,'author':string,'content':string,'link':string,'date':int,'hash':string,
+	/** @param array{'id':string,'guid':string,'title':string,'author':string,'content':string,'link':string,'date':int,'lastSeen':int,'hash':string,
 	 *		'is_read':bool|int|null,'is_favorite':bool|int|null,'id_feed':int,'tags':string,'attributes':array<string,mixed>} $valuesTmp */
 	public function addEntry(array $valuesTmp, bool $useTmpTable = true): bool {
 		if ($this->addEntryPrepared == null) {
@@ -556,7 +556,10 @@ SQL;
 			$idMax = time() . '000000';
 			Minz_Log::debug('Calling markReadFeed(0) is deprecated!');
 		}
-		$this->pdo->beginTransaction();
+		$hadTransaction = $this->pdo->inTransaction();
+		if (!$hadTransaction) {
+			$this->pdo->beginTransaction();
+		}
 
 		$sql = 'UPDATE `_entry` '
 			 . 'SET is_read=? '
@@ -589,7 +592,9 @@ SQL;
 			}
 		}
 
-		$this->pdo->commit();
+		if (!$hadTransaction) {
+			$this->pdo->commit();
+		}
 		return $affected;
 	}
 
@@ -698,7 +703,7 @@ SQL;
 		}
 	}
 
-	/** @return Traversable<array{'id':string,'guid':string,'title':string,'author':string,'content':string,'link':string,'date':int,
+	/** @return Traversable<array{'id':string,'guid':string,'title':string,'author':string,'content':string,'link':string,'date':int,'lastSeen':int,
 	 *		'hash':string,'is_read':?bool,'is_favorite':?bool,'id_feed':int,'tags':string,'attributes':array<string,mixed>}> */
 	public function selectAll(): Traversable {
 		$content = static::isCompressed() ? 'UNCOMPRESS(content_bin) AS content' : 'content';

app/Models/Feed.php

@@ -764,23 +764,35 @@ class FreshRSS_Feed extends Minz_Model {
 
 	/**
 	 * Applies the *mark as read upon gone* policy, if enabled.
-	 * Remember to call updateCachedValue($id_feed) or updateCachedValues() just after.
+	 * Remember to call `updateCachedValue($id_feed)` or `updateCachedValues()` just after.
 	 * @return int|false the number of lines affected, or false if not applicable
 	 */
-	public function markAsReadUponGone() {
+	public function markAsReadUponGone(bool $upstreamIsEmpty, int $maxTimestamp = 0) {
 		$readUponGone = $this->attributes('read_upon_gone');
 		if ($readUponGone === null) {
 			$readUponGone = FreshRSS_Context::$user_conf->mark_when['gone'];
 		}
-		if ($readUponGone) {
+		if (!$readUponGone) {
+			return false;
+		}
+		if ($upstreamIsEmpty) {
+			if ($maxTimestamp <= 0) {
+				$maxTimestamp = time();
+			}
+			$entryDAO = FreshRSS_Factory::createEntryDao();
+			$affected = $entryDAO->markReadFeed($this->id(), $maxTimestamp . '000000');
+		} else {
 			$feedDAO = FreshRSS_Factory::createFeedDao();
-			return $feedDAO->markAsReadUponGone($this->id());
+			$affected = $feedDAO->markAsReadUponGone($this->id());
 		}
-		return false;
+		if ($affected > 0) {
+			Minz_Log::debug(__METHOD__ . " $affected items" . ($upstreamIsEmpty ? ' (all)' : '') . ' [' . $this->url(false) . ']');
+		}
+		return $affected;
 	}
 
 	/**
-	 * Remember to call updateCachedValue($id_feed) or updateCachedValues() just after
+	 * Remember to call `updateCachedValue($id_feed)` or `updateCachedValues()` just after
 	 * @return int|false
 	 */
 	public function cleanOldEntries() {

app/Models/FeedDAO.php

@@ -503,16 +503,12 @@ WHERE id_feed=:id_feed1 AND is_read=0 AND (
 	`lastSeen` + 60 < (SELECT s1.maxlastseen FROM (
 		SELECT MAX(e2.`lastSeen`) AS maxlastseen FROM `_entry` e2 WHERE e2.id_feed = :id_feed2
 	) s1)
-	OR `lastSeen` + 60 < (SELECT s2.lastcorrectupdate FROM (
-		SELECT f2.`lastUpdate` AS lastcorrectupdate FROM `_feed` f2 WHERE f2.id = :id_feed3 AND f2.error = 0
-	) s2)
 )
 SQL;
 
 		if (($stm = $this->pdo->prepare($sql)) &&
 			$stm->bindParam(':id_feed1', $id, PDO::PARAM_INT) &&
 			$stm->bindParam(':id_feed2', $id, PDO::PARAM_INT) &&
-			$stm->bindParam(':id_feed3', $id, PDO::PARAM_INT) &&
 			$stm->execute()) {
 			return $stm->rowCount();
 		} else {