diff --git a/changedetectionio/model/Watch.py b/changedetectionio/model/Watch.py index 34062df0d..29be66bfa 100644 --- a/changedetectionio/model/Watch.py +++ b/changedetectionio/model/Watch.py @@ -867,15 +867,30 @@ class model(EntityPersistenceMixin, watch_base): if not favicon_fname: return True try: - fname = next(iter(glob.glob(os.path.join(self.data_dir, "favicon.*"))), None) - logger.trace(f"Favicon file maybe found at {fname}") - if os.path.isfile(fname): - file_age = int(time.time() - os.path.getmtime(fname)) - logger.trace(f"Favicon file age is {file_age}s") - if file_age < FAVICON_RESAVE_THRESHOLD_SECONDS: - return False + # Newest by mtime rather than whatever the directory happens to list first. + # Older versions of bump_favicon() left any previous save under a different + # extension in place, so a watch whose icon changed type can have more than one + # favicon.* file. glob order is os.scandir order - stable, but decided by + # filename hash - so for some extension pairs the stale file wins every single + # time, the age check always fails, and the favicon is refetched on every check + # forever. Measured: 3 of 10 realistic extension pairs lose that way. + candidates = glob.glob(os.path.join(self.data_dir, "favicon.*")) + if not candidates: + # get_favicon_filename() is served from a module-level cache, so it can say + # yes after the file has been removed. Not an error, just a refetch. + logger.trace(f"No favicon file in {self.data_dir}, treating as expired") + return True + fname = max(candidates, key=os.path.getmtime) + logger.trace(f"Favicon file found at {fname}") + file_age = int(time.time() - os.path.getmtime(fname)) + logger.trace(f"Favicon file age is {file_age}s") + if file_age < FAVICON_RESAVE_THRESHOLD_SECONDS: + return False except Exception as e: - logger.critical(f"Exception checking Favicon age {str(e)}") + # Deliberately broad: this runs inline on the check path (it is an argument to + # fetcher.run()), so it must never be the reason a watch fails. Not critical + # either - the only cost of getting it wrong is one extra favicon fetch. + logger.warning(f"Could not check favicon age in {self.data_dir}, will refetch: {e}") return True # Also in the case that the file didnt exist @@ -943,6 +958,19 @@ class model(EntityPersistenceMixin, watch_base): with open(fname, 'wb') as f: f.write(decoded) + # Drop any previous save under a different extension, or the watch ends up + # holding several favicon.* files and favicon_is_expired() has to guess which one + # describes the current icon. Only after the write, so a failed write keeps the old icon. + import glob + for stale in glob.glob(os.path.join(self.data_dir, "favicon.*")): + if os.path.abspath(stale) == os.path.abspath(fname): + continue + try: + os.unlink(stale) + logger.debug(f"UUID: {self.get('uuid')} removed superseded favicon {stale}") + except OSError as e: + logger.debug(f"UUID: {self.get('uuid')} could not remove {stale}: {e}") + # Invalidate module-level favicon filename cache for this watch _FAVICON_FILENAME_CACHE.pop(self.data_dir, None) diff --git a/changedetectionio/tests/unit/test_watch_model.py b/changedetectionio/tests/unit/test_watch_model.py index 1e0284cd8..a73bb49f8 100644 --- a/changedetectionio/tests/unit/test_watch_model.py +++ b/changedetectionio/tests/unit/test_watch_model.py @@ -5,6 +5,9 @@ import unittest import os +import time +import glob +import base64 import pickle import tempfile from copy import deepcopy @@ -279,6 +282,82 @@ class TestFaviconFilenameCache(unittest.TestCase): self.assertIsNone(watch.get_favicon_filename()) +class TestFaviconExpiry(unittest.TestCase): + """favicon_is_expired() must judge the icon the watch actually has now.""" + + def _watch(self, datastore_path): + from changedetectionio.model.Watch import _FAVICON_FILENAME_CACHE + watch = Watch.model( + datastore_path=datastore_path, + __datastore={'settings': {'application': {}}, 'watching': {}}, + default={'url': 'https://example.com'} + ) + watch.ensure_data_dir_exists() + self.addCleanup(_FAVICON_FILENAME_CACHE.pop, watch.data_dir, None) + return watch + + def test_fresh_favicon_wins_over_a_stale_one_of_another_extension(self): + """A leftover favicon. must not force a refetch on every check. + + bump_favicon() names the file after the icon's type, so a watch whose icon changed + type could hold two. glob order is decided by filename hash, so picking the first + match meant some extension pairs always chose the stale file - the age check failed + every time and the favicon was refetched forever. + """ + with tempfile.TemporaryDirectory() as datastore_path: + watch = self._watch(datastore_path) + + stale = os.path.join(watch.data_dir, 'favicon.png') + with open(stale, 'wb') as f: + f.write(b'stale') + old = time.time() - (40 * 86400) + os.utime(stale, (old, old)) + + fresh = os.path.join(watch.data_dir, 'favicon.ico') + with open(fresh, 'wb') as f: + f.write(b'fresh') + + self.assertFalse(watch.favicon_is_expired(), + "a favicon saved seconds ago must not count as expired just " + "because an older one of a different extension is also present") + + def test_expired_when_the_newest_favicon_is_old(self): + with tempfile.TemporaryDirectory() as datastore_path: + watch = self._watch(datastore_path) + path = os.path.join(watch.data_dir, 'favicon.ico') + with open(path, 'wb') as f: + f.write(b'old') + old = time.time() - (40 * 86400) + os.utime(path, (old, old)) + + self.assertTrue(watch.favicon_is_expired()) + + def test_no_favicon_file_is_expired_and_does_not_raise(self): + """The filename cache can say yes after the file is gone; that is a refetch, not an error.""" + from changedetectionio.model.Watch import _FAVICON_FILENAME_CACHE + with tempfile.TemporaryDirectory() as datastore_path: + watch = self._watch(datastore_path) + _FAVICON_FILENAME_CACHE[watch.data_dir] = 'favicon.ico' # stale positive + + self.assertTrue(watch.favicon_is_expired()) + + def test_bump_favicon_removes_the_superseded_file(self): + with tempfile.TemporaryDirectory() as datastore_path: + watch = self._watch(datastore_path) + previous = os.path.join(watch.data_dir, 'favicon.ico') + with open(previous, 'wb') as f: + f.write(b'previous') + + watch.bump_favicon(url='https://example.com/icon.png', + favicon_base_64=base64.b64encode(b'newicon').decode(), + mime_type='image/png') + + remaining = sorted(os.path.basename(x) for x in + glob.glob(os.path.join(watch.data_dir, 'favicon.*'))) + self.assertEqual(remaining, ['favicon.png'], + "the old favicon.ico should have been removed") + + class TestLLMDiffSummaryCache(unittest.TestCase): """Tests for get_llm_diff_summary / save_llm_diff_summary — version-pair + prompt-hash caching."""