fix(logos): stop a failed download pinning a team to a grey box forever

src/common/logo_helper.py

@@ -143,22 +143,72 @@ def load_logo_with_download(self, team_abbr: str, logo_path: Union[str, Path],
         """
         logo_path = Path(logo_path)
 
-        # Try to load existing logo first
-        if logo_path.exists():
+        # Try to load existing logo first. A placeholder written by a previous
+        # failed download does not count: it wears the real logo's filename, so
+        # trusting the file's existence is what left teams as grey boxes.
+        if logo_path.exists() and not self._is_stale_placeholder(logo_path):
             return self.load_logo(team_abbr, logo_path, max_width, max_height)
 
         # Download if URL provided and file doesn't exist
         if logo_url:
             try:
                 self.logger.info(f"Downloading logo for {team_abbr} from {logo_url}")
                 self._download_logo(logo_url, logo_path)
+                # The file on disk just changed. Any cached image for it is the
+                # placeholder we came here to replace, and load_logo() answers
+                # from the cache before touching the disk -- so without this the
+                # real logo would not appear until the process restarted.
+                self._invalidate_cached_logo(team_abbr, logo_path)
                 return self.load_logo(team_abbr, logo_path, max_width, max_height)
             except Exception as e:
                 self.logger.error(f"Failed to download logo for {team_abbr}: {e}")
+                # The retry failed, so restart the back-off. The stale
+                # placeholder is still on disk with its old timestamp, and
+                # leaving it there means the next call retries immediately --
+                # a download attempt per call, which is what the back-off
+                # exists to prevent.
+                self._refresh_stale_placeholder(logo_path)
 
         # Create placeholder if all else fails
         return self._create_placeholder_logo(team_abbr, max_width, max_height)
 
+    def _invalidate_cached_logo(self, team_abbr: str, logo_path: Path) -> None:
+        """Drop every cached size of one logo after its file changed on disk."""
+        prefix = f"{team_abbr}_{logo_path}_"
+        for key in [k for k in self._logo_cache if k.startswith(prefix)]:
+            self._logo_cache.pop(key, None)
+            if key in self._cache_order:
+                self._cache_order.remove(key)
+
+    @staticmethod
+    def _refresh_stale_placeholder(logo_path: Path) -> None:
+        """Restart the retry back-off after a failed download attempt."""
+        try:
+            from src.logo_downloader import refresh_placeholder_timestamp
+        except ImportError:
+            return
+        refresh_placeholder_timestamp(logo_path)
+
+    @staticmethod
+    def _is_stale_placeholder(logo_path: Path) -> bool:
+        """True if the file is a placeholder old enough to be worth retrying.
+
+        Imported lazily so this module keeps working against a core build whose
+        logo_downloader predates placeholder marking.
+        """
+        try:
+            from src.logo_downloader import (
+                PLACEHOLDER_RETRY_SECONDS,
+                is_placeholder_logo,
+                placeholder_age_seconds,
+            )
+        except ImportError:
+            return False
+        if not is_placeholder_logo(logo_path):
+            return False
+        age = placeholder_age_seconds(logo_path)
+        return age is None or age >= PLACEHOLDER_RETRY_SECONDS
+
     def get_logo_variations(self, team_abbr: str) -> List[str]:
         """
         Get possible filename variations for a team abbreviation.

src/logo_downloader.py

@@ -14,6 +14,7 @@
 from typing import Dict, List, Optional, Tuple
 from pathlib import Path
 from PIL import Image, ImageDraw, ImageFont
+from PIL.PngImagePlugin import PngInfo
 from requests.adapters import HTTPAdapter
 from urllib3.util.retry import Retry
 from src.common.permission_utils import (
@@ -25,6 +26,96 @@
 
 logger = logging.getLogger(__name__)
 
+#: PNG text key stamped into a generated placeholder so a later run can tell it
+#: apart from a real logo that happens to be small.
+PLACEHOLDER_MARKER = "ledmatrix_placeholder"
+
+#: Geometry of a generated placeholder, used to recognise ones written before
+#: the marker existed. Those are already on users' disks and would otherwise
+#: never be retried.
+PLACEHOLDER_SIZE = (64, 64)
+PLACEHOLDER_BG = (100, 100, 100, 255)
+
+#: How long a placeholder is trusted before the real logo is attempted again.
+#: A placeholder means the download failed, and download failures are usually
+#: transient (no network at boot, ESPN blipping). Retrying every frame would
+#: hammer the API from a Pi that is also driving a panel; never retrying leaves
+#: the team a grey box forever, which is the bug this exists to avoid.
+PLACEHOLDER_RETRY_SECONDS = 6 * 60 * 60
+
+
+def is_placeholder_logo(filepath: Path) -> bool:
+    """True if the file at ``filepath`` is a generated placeholder, not a logo.
+
+    Checks the marker first, then falls back to matching the placeholder's
+    exact geometry and background colour so files written before the marker was
+    introduced are still recognised.
+    """
+    try:
+        with Image.open(filepath) as img:
+            if img.info.get(PLACEHOLDER_MARKER):
+                return True
+            if img.size != PLACEHOLDER_SIZE:
+                return False
+            return img.convert("RGBA").getpixel((0, 0)) == PLACEHOLDER_BG
+    except Exception:
+        # Unreadable file: not provably a placeholder, and the caller's own
+        # error handling is better placed to deal with it.
+        return False
+
+
+def should_attempt_download(filepath: Path, force_download: bool = False) -> bool:
+    """Whether a real logo is worth (re)fetching for ``filepath``.
+
+    True when nothing is there, when the caller forced it, or when what is
+    there is a placeholder old enough to retry. A *fresh* placeholder says a
+    download just failed, so retrying it immediately would hammer the API for
+    a result that is very unlikely to have changed.
+    """
+    if force_download or not filepath.exists():
+        return True
+    if not is_placeholder_logo(filepath):
+        return False
+    age = placeholder_age_seconds(filepath)
+    return age is None or age >= PLACEHOLDER_RETRY_SECONDS
+
+
+def refresh_placeholder_timestamp(filepath: Path) -> bool:
+    """Restamp a placeholder so a failed retry restarts the back-off clock.
+
+    Without this a stale placeholder stays stale: every later call sees an
+    expired timestamp, retries, fails, and leaves the timestamp untouched --
+    which is a download attempt per call, the opposite of what the back-off is
+    for.
+    """
+    try:
+        if not is_placeholder_logo(filepath):
+            return False
+        metadata = PngInfo()
+        metadata.add_text(PLACEHOLDER_MARKER, str(time.time()))
+        with Image.open(filepath) as img:
+            img.copy().save(filepath, "PNG", pnginfo=metadata)
+        return True
+    except Exception:
+        logger.debug("Could not refresh placeholder timestamp for %s", filepath,
+                     exc_info=True)
+        return False
+
+
+def placeholder_age_seconds(filepath: Path) -> Optional[float]:
+    """Seconds since a placeholder was written, or None if unknown."""
+    try:
+        with Image.open(filepath) as img:
+            stamped = img.info.get(PLACEHOLDER_MARKER)
+        if stamped and stamped != "1":
+            return max(0.0, time.time() - float(stamped))
+    except Exception:
+        pass
+    try:
+        return max(0.0, time.time() - filepath.stat().st_mtime)
+    except OSError:
+        return None
+
 class LogoDownloader:
     """Centralized logo downloader for team logos from ESPN API."""
 
@@ -499,8 +590,10 @@
             filename = f"{self.normalize_abbreviation(abbreviation)}.png"
             filepath = Path(logo_dir) / filename
 
-            # Skip if already exists and not forcing download
-            if filepath.exists() and not force_download:
+            # A placeholder does not count as existing -- it is a previous
+            # failure, and a bulk pass is exactly where it should get another
+            # chance, subject to the same back-off as everywhere else.
+            if not should_attempt_download(filepath, force_download):
                 logger.debug(f"Skipping {display_name}: {filename} already exists")
                 continue
 
@@ -559,8 +652,9 @@
             filename = f"{self.normalize_abbreviation(abbreviation)}.png"
             filepath = Path(logo_dir) / filename
 
-            # Skip if already exists and not forcing download
-            if filepath.exists() and not force_download:
+            # Same eligibility rule as every other download site: a stale
+            # placeholder is a failed download, not a logo.
+            if not should_attempt_download(filepath, force_download):
                 logger.debug(f"Skipping {display_name} ({category}, {conference}): {filename} already exists")
                 continue
 
@@ -674,11 +768,16 @@
                 # Fallback without font
                 draw.text((16, 24), text, fill=(255, 255, 255, 255))
 
-            logo.save(filepath)
-
+            # Stamp it so a later run can tell this apart from a real logo and
+            # retry the download, instead of treating the file's existence as
+            # proof the logo was fetched.
+            metadata = PngInfo()
+            metadata.add_text(PLACEHOLDER_MARKER, str(time.time()))
+            logo.save(filepath, "PNG", pnginfo=metadata)
+
             # Set proper file permissions after saving
             ensure_file_permissions(filepath, get_assets_file_mode())
-            
+
             logger.info(f"Created placeholder logo for {team_abbreviation} at {filepath}")
             return True
 
@@ -770,9 +869,18 @@
     # Use the exact filepath that was passed in (respects config settings)
     filepath = logo_path
 
-    if filepath.exists():
+    if filepath.exists() and not should_attempt_download(filepath):
+        # Either a real logo, or a placeholder too fresh to be worth retrying.
         logger.debug(f"Logo already exists for {team_abbreviation} ({league})")
         return True
+    if filepath.exists():
+        # A placeholder is a *failed* download wearing the real logo's
+        # filename. Treating it as "already exists" is what pinned a team to a
+        # grey box permanently after one transient failure.
+        logger.info(
+            "Logo for %s (%s) is a placeholder from a failed download; "
+            "retrying the real logo", team_abbreviation, league,
+        )
 
     # Try to download the real logo first
     logger.info(f"Attempting to download logo for {team_abbreviation} from {league}")