Judge favicon expiry on the icon the watch actually has, not whatever glob lists first

bump_favicon() names the saved file after the icon's type (favicon.png, favicon.ico, ...)
and did not remove a previous save under a different extension, so a watch whose icon
changed type ends up holding more than one favicon.*.

favicon_is_expired() then picked next(iter(glob.glob("favicon.*"))). glob order is
os.scandir order - stable, but decided by filename hash - so for some extension pairs the
stale file wins every single time, its age always exceeds the 24h threshold, and the
favicon is refetched on every check forever. Measured across realistic pairs, 3 of 10
lose that way:

  old=.png new=.ico   -> glob lists favicon.png first  -> always expired
  old=.ico new=.svg   -> glob lists favicon.ico first  -> always expired
  old=.png new=.webp  -> glob lists favicon.png first  -> always expired

That is an in-page network fetch through the watch's proxy on every check, plus the
favicon fetcher's own sequential per-icon timeouts, for a favicon that is already saved
and current.

Fixed at both ends: pick the newest candidate by mtime, and have bump_favicon() drop
superseded files so the ambiguity stops arising.

Also tidied two things in the same function:

- os.path.isfile(None) raised a TypeError when the module-level filename cache said yes
  after the file had been removed, which the broad except turned into a logger.critical()
  for an entirely benign condition. The empty case is now handled explicitly.
- the broad except stays (this runs inline on the check path as an argument to
  fetcher.run(), so it must never fail a watch) but logs at warning, since the only cost
  of getting this wrong is one extra favicon fetch.

Tests fail on master (favicon.ico left behind; fresh icon reported expired) and pass here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

changedetectionio/model/Watch.py

@@ -848,15 +848,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.
+            # bump_favicon() writes favicon.<ext> and (before this) left any previous save
+            # under a different extension in place, so a watch whose icon changed type has
+            # more than one 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
@@ -921,6 +936,19 @@ class model(EntityPersistenceMixin, watch_base):
             return None
 
         try:
+            # Drop any previous save under a different extension first, or the watch ends up
+            # holding several favicon.* files and favicon_is_expired() has to guess which one
+            # describes the current icon.
+            import glob as _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}")
+
             with open(fname, 'wb') as f:
                 f.write(decoded)
 

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.<other ext> 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."""