mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 22:35:08 +00:00
fix(logos): stop a failed download pinning a team to a grey box forever (#512)
* fix(logos): stop a failed download pinning a team to a grey box forever When a logo download fails, create_placeholder_logo writes a 64x64 grey PNG under the *real* logo's filename. Every later call then hits `if filepath.exists(): return True` and reports success, so the real logo is never attempted again. One transient failure -- no network at boot, ESPN blipping -- permanently costs that team its logo. This is not hypothetical. Five of the eleven cached AFL logos in my checkout were 384-byte stubs written in a single bad minute, and they had stayed that way ever since; the scoreboard rendered COLL, FRE, NMFC, PORT and SYD as grey text boxes on every card. Placeholders are now stamped with a `ledmatrix_placeholder` PNG text chunk carrying their creation time, and `is_placeholder_logo` recognises them. It also matches on the placeholder's exact geometry and background colour, so the stubs already sitting on users' disks are picked up too -- without that, this fix would only help teams whose logos break in future. Verified against the real stubs: all five detected, all six real logos untouched. `download_missing_logo` now treats an existing placeholder as the failed download it is and retries, rather than as a satisfied request. The retry is rate-limited to PLACEHOLDER_RETRY_SECONDS (6h) so this does not trade a permanent grey box for an ESPN request every frame; a failed retry rewrites the placeholder, restarting the clock. The age comes from the stamp rather than mtime, so a backup restore, an rsync, or a permissions script cannot silently reset it. `download_missing_logos_for_league` gets the same treatment -- a bulk pass is exactly where a previously failed logo should get another chance -- and `LogoHelper.load_logo_with_download` no longer accepts a stale placeholder as a cache hit. That import is lazy and guarded so the module still works against a core build predating the marker. `LogoHelper._create_placeholder_logo` needs no change: it returns an in-memory image and never writes it to disk, which is the behaviour this bug argues for. Tests cover marked and legacy-unmarked detection, the two false-positive cases (a real 500x500 logo, and a 64x64 image that is merely the same size), the retry, the rate limit, and that the age survives an mtime touch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(logos): address review — unify eligibility, invalidate cache, restart back-off Three findings from the review on #512, all confirmed against the code: 1. The three download sites each had their own idea of "already have it". download_missing_logos_for_league() retried *any* placeholder, ignoring the back-off entirely, while download_all_ncaa_football_logos() was never updated and still skipped placeholders forever. They now share one should_attempt_download(), which also covers force_download, so the sites cannot drift apart again. download_missing_logo() reads through the same helper. 2. LogoHelper.load_logo_with_download() answered from the in-memory cache before touching the disk, so after a stale placeholder was successfully replaced the *cached placeholder image* was still returned -- the real logo would not have appeared until the process restarted. The cache entry for that file (every size of it) is now dropped after a successful download. 3. A failed retry left the stale placeholder on disk with its old timestamp, so the next call saw it as stale again and retried immediately: a download attempt per call, which is precisely what the back-off exists to prevent. refresh_placeholder_timestamp() restamps it, and the helper calls that on the failure path. It refuses to touch anything that is not a placeholder. Tests cover both bulk loops in both directions (fresh placeholder skipped, stale one retried), the eligibility rule including force_download, the timestamp refresh, and the two LogoHelper paths -- including that a freshly-downloaded logo is actually what comes back rather than the cached placeholder. Two of the new bulk-loop tests initially passed for the wrong reason: the fetch_teams_data stub returned {}, which is falsy, so the loops bailed before reaching the eligibility check at all. Fixed to return a truthy payload. Re-verified end to end: with both halves in place, rendering the AFL scoreboard took FRE.png from a 362-byte stub to a 12,928-byte logo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Chuck
@@ -8,11 +8,27 @@ ensure_logo_directory, and the download_missing_logo function path
|
||||
"""
|
||||
|
||||
import os
|
||||
import time
|
||||
|
||||
import pytest
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch, Mock, MagicMock
|
||||
|
||||
from src.logo_downloader import LogoDownloader
|
||||
from PIL import Image
|
||||
from PIL.PngImagePlugin import PngInfo
|
||||
|
||||
from src.logo_downloader import (
|
||||
PLACEHOLDER_BG,
|
||||
PLACEHOLDER_MARKER,
|
||||
PLACEHOLDER_RETRY_SECONDS,
|
||||
PLACEHOLDER_SIZE,
|
||||
LogoDownloader,
|
||||
download_missing_logo,
|
||||
is_placeholder_logo,
|
||||
placeholder_age_seconds,
|
||||
refresh_placeholder_timestamp,
|
||||
should_attempt_download,
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -127,3 +143,211 @@ class TestEnsureLogoDirectory:
|
||||
with patch("builtins.open", side_effect=mock_open):
|
||||
result = downloader.ensure_logo_directory(test_dir)
|
||||
assert result is False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Placeholder detection and retry
|
||||
#
|
||||
# A failed download used to be cached as a placeholder wearing the real logo's
|
||||
# filename, and download_missing_logo returned early on "the file exists". One
|
||||
# transient failure therefore pinned a team to a grey box permanently.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestPlaceholderLogos:
|
||||
def _placeholder(self, tmp_path, abbrev="COLL"):
|
||||
downloader = LogoDownloader()
|
||||
assert downloader.create_placeholder_logo(abbrev, str(tmp_path)) is True
|
||||
return tmp_path / f"{abbrev}.png"
|
||||
|
||||
def test_generated_placeholder_is_recognised(self, tmp_path):
|
||||
assert is_placeholder_logo(self._placeholder(tmp_path)) is True
|
||||
|
||||
def test_real_logo_is_not_a_placeholder(self, tmp_path):
|
||||
real = tmp_path / "REAL.png"
|
||||
Image.new("RGBA", (500, 500), (12, 34, 56, 255)).save(real)
|
||||
assert is_placeholder_logo(real) is False
|
||||
|
||||
def test_legacy_unmarked_placeholder_is_recognised(self, tmp_path):
|
||||
"""Placeholders written before the marker existed must still be caught.
|
||||
|
||||
They are already sitting on users' disks; if they were not recognised
|
||||
those teams would stay grey boxes forever even after this fix.
|
||||
"""
|
||||
legacy = tmp_path / "LEGACY.png"
|
||||
Image.new("RGBA", PLACEHOLDER_SIZE, PLACEHOLDER_BG).save(legacy)
|
||||
assert is_placeholder_logo(legacy) is True
|
||||
|
||||
def test_same_size_but_different_colour_is_not_a_placeholder(self, tmp_path):
|
||||
real = tmp_path / "SMALL.png"
|
||||
Image.new("RGBA", PLACEHOLDER_SIZE, (10, 200, 10, 255)).save(real)
|
||||
assert is_placeholder_logo(real) is False
|
||||
|
||||
def test_missing_file_is_not_a_placeholder(self, tmp_path):
|
||||
assert is_placeholder_logo(tmp_path / "nope.png") is False
|
||||
|
||||
def test_existing_real_logo_short_circuits_without_downloading(self, tmp_path):
|
||||
real = tmp_path / "REAL.png"
|
||||
Image.new("RGBA", (500, 500), (1, 2, 3, 255)).save(real)
|
||||
with patch.object(LogoDownloader, "download_logo") as download:
|
||||
assert download_missing_logo(
|
||||
"afl", "1", "REAL", real, logo_url="http://example/x.png") is True
|
||||
download.assert_not_called()
|
||||
|
||||
def _age_placeholder(self, path, seconds):
|
||||
"""Rewrite a placeholder's marker so it reads as `seconds` old."""
|
||||
metadata = PngInfo()
|
||||
metadata.add_text(PLACEHOLDER_MARKER, str(time.time() - seconds))
|
||||
with Image.open(path) as img:
|
||||
img.copy().save(path, "PNG", pnginfo=metadata)
|
||||
|
||||
def test_stale_placeholder_triggers_a_retry(self, tmp_path):
|
||||
path = self._placeholder(tmp_path)
|
||||
self._age_placeholder(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
||||
assert placeholder_age_seconds(path) > PLACEHOLDER_RETRY_SECONDS
|
||||
|
||||
with patch.object(LogoDownloader, "download_logo", return_value=True) as download:
|
||||
assert download_missing_logo(
|
||||
"afl", "1", "COLL", path,
|
||||
logo_url="http://example/coll.png") is True
|
||||
download.assert_called_once()
|
||||
|
||||
def test_placeholder_age_survives_an_mtime_touch(self, tmp_path):
|
||||
"""The age comes from the stamp, not the filesystem.
|
||||
|
||||
Anything that rewrites file times -- a backup restore, an rsync, a
|
||||
permissions fix script -- would otherwise reset the retry clock.
|
||||
"""
|
||||
path = self._placeholder(tmp_path)
|
||||
self._age_placeholder(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
||||
now = time.time()
|
||||
os.utime(path, (now, now))
|
||||
assert placeholder_age_seconds(path) > PLACEHOLDER_RETRY_SECONDS
|
||||
|
||||
def test_fresh_placeholder_does_not_retry(self, tmp_path):
|
||||
"""Rate limiting: a placeholder written seconds ago must not re-download.
|
||||
|
||||
Without this the fix would trade a permanent grey box for an ESPN
|
||||
request on every frame.
|
||||
"""
|
||||
path = self._placeholder(tmp_path)
|
||||
with patch.object(LogoDownloader, "download_logo") as download:
|
||||
assert download_missing_logo(
|
||||
"afl", "1", "COLL", path,
|
||||
logo_url="http://example/coll.png") is True
|
||||
download.assert_not_called()
|
||||
|
||||
|
||||
class TestDownloadEligibility:
|
||||
"""One rule, shared by every download site.
|
||||
|
||||
The two bulk loops and the single-logo path each had their own idea of what
|
||||
counted as "already have it", which is how one of them ended up retrying
|
||||
fresh placeholders and the other skipping stale ones forever.
|
||||
"""
|
||||
|
||||
def _placeholder(self, tmp_path, abbrev="COLL"):
|
||||
assert LogoDownloader().create_placeholder_logo(abbrev, str(tmp_path))
|
||||
return tmp_path / f"{abbrev}.png"
|
||||
|
||||
def _age(self, path, seconds):
|
||||
metadata = PngInfo()
|
||||
metadata.add_text(PLACEHOLDER_MARKER, str(time.time() - seconds))
|
||||
with Image.open(path) as img:
|
||||
img.copy().save(path, "PNG", pnginfo=metadata)
|
||||
|
||||
def test_missing_file_is_eligible(self, tmp_path):
|
||||
assert should_attempt_download(tmp_path / "nope.png") is True
|
||||
|
||||
def test_real_logo_is_not_eligible(self, tmp_path):
|
||||
real = tmp_path / "REAL.png"
|
||||
Image.new("RGBA", (500, 500), (1, 2, 3, 255)).save(real)
|
||||
assert should_attempt_download(real) is False
|
||||
|
||||
def test_force_download_beats_a_real_logo(self, tmp_path):
|
||||
real = tmp_path / "REAL.png"
|
||||
Image.new("RGBA", (500, 500), (1, 2, 3, 255)).save(real)
|
||||
assert should_attempt_download(real, force_download=True) is True
|
||||
|
||||
def test_fresh_placeholder_is_not_eligible(self, tmp_path):
|
||||
assert should_attempt_download(self._placeholder(tmp_path)) is False
|
||||
|
||||
def test_stale_placeholder_is_eligible(self, tmp_path):
|
||||
path = self._placeholder(tmp_path)
|
||||
self._age(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
||||
assert should_attempt_download(path) is True
|
||||
|
||||
def test_league_bulk_loop_skips_a_fresh_placeholder(self, tmp_path):
|
||||
"""A bulk pass honours the same back-off as everything else."""
|
||||
self._placeholder(tmp_path, "AAA")
|
||||
downloader = LogoDownloader()
|
||||
teams = [{"abbreviation": "AAA", "display_name": "A", "logo_url": "http://x/a.png"}]
|
||||
with patch.object(LogoDownloader, "get_logo_directory", return_value=str(tmp_path)):
|
||||
with patch.object(LogoDownloader, "fetch_teams_data", return_value={"sports": [{}]}):
|
||||
with patch.object(LogoDownloader, "extract_teams_from_data", return_value=teams):
|
||||
with patch.object(LogoDownloader, "download_logo") as download:
|
||||
downloader.download_missing_logos_for_league("nfl")
|
||||
download.assert_not_called()
|
||||
|
||||
def test_league_bulk_loop_retries_a_stale_placeholder(self, tmp_path):
|
||||
path = self._placeholder(tmp_path, "AAA")
|
||||
self._age(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
||||
downloader = LogoDownloader()
|
||||
teams = [{"abbreviation": "AAA", "display_name": "A", "logo_url": "http://x/a.png"}]
|
||||
with patch.object(LogoDownloader, "get_logo_directory", return_value=str(tmp_path)):
|
||||
with patch.object(LogoDownloader, "fetch_teams_data", return_value={"sports": [{}]}):
|
||||
with patch.object(LogoDownloader, "extract_teams_from_data", return_value=teams):
|
||||
with patch.object(LogoDownloader, "download_logo", return_value=True) as download:
|
||||
downloader.download_missing_logos_for_league("nfl")
|
||||
download.assert_called_once()
|
||||
|
||||
def test_ncaa_bulk_loop_retries_a_stale_placeholder(self, tmp_path):
|
||||
"""This loop skipped placeholders forever; it now shares the rule."""
|
||||
path = self._placeholder(tmp_path, "AAA")
|
||||
self._age(path, PLACEHOLDER_RETRY_SECONDS + 60)
|
||||
downloader = LogoDownloader()
|
||||
teams = [{"abbreviation": "AAA", "display_name": "A",
|
||||
"logo_url": "http://x/a.png", "category": "FBS",
|
||||
"conference": "SEC"}]
|
||||
with patch.object(LogoDownloader, "get_logo_directory", return_value=str(tmp_path)):
|
||||
with patch.object(LogoDownloader, "fetch_teams_data", return_value={"sports": [{}]}):
|
||||
with patch.object(LogoDownloader, "extract_teams_from_data", return_value=teams):
|
||||
with patch.object(LogoDownloader, "download_logo", return_value=True) as download:
|
||||
downloader.download_all_ncaa_football_logos()
|
||||
download.assert_called_once()
|
||||
|
||||
def test_ncaa_bulk_loop_skips_a_fresh_placeholder(self, tmp_path):
|
||||
self._placeholder(tmp_path, "AAA")
|
||||
downloader = LogoDownloader()
|
||||
teams = [{"abbreviation": "AAA", "display_name": "A",
|
||||
"logo_url": "http://x/a.png", "category": "FBS",
|
||||
"conference": "SEC"}]
|
||||
with patch.object(LogoDownloader, "get_logo_directory", return_value=str(tmp_path)):
|
||||
with patch.object(LogoDownloader, "fetch_teams_data", return_value={"sports": [{}]}):
|
||||
with patch.object(LogoDownloader, "extract_teams_from_data", return_value=teams):
|
||||
with patch.object(LogoDownloader, "download_logo") as download:
|
||||
downloader.download_all_ncaa_football_logos()
|
||||
download.assert_not_called()
|
||||
|
||||
|
||||
class TestRefreshPlaceholderTimestamp:
|
||||
def test_restarts_the_back_off(self, tmp_path):
|
||||
assert LogoDownloader().create_placeholder_logo("COLL", str(tmp_path))
|
||||
path = tmp_path / "COLL.png"
|
||||
metadata = PngInfo()
|
||||
metadata.add_text(PLACEHOLDER_MARKER, str(time.time() - (PLACEHOLDER_RETRY_SECONDS + 60)))
|
||||
with Image.open(path) as img:
|
||||
img.copy().save(path, "PNG", pnginfo=metadata)
|
||||
assert should_attempt_download(path) is True
|
||||
|
||||
assert refresh_placeholder_timestamp(path) is True
|
||||
assert should_attempt_download(path) is False
|
||||
|
||||
def test_refuses_to_touch_a_real_logo(self, tmp_path):
|
||||
real = tmp_path / "REAL.png"
|
||||
Image.new("RGBA", (500, 500), (1, 2, 3, 255)).save(real)
|
||||
before = real.read_bytes()
|
||||
assert refresh_placeholder_timestamp(real) is False
|
||||
assert real.read_bytes() == before
|
||||
|
||||
def test_missing_file_is_not_an_error(self, tmp_path):
|
||||
assert refresh_placeholder_timestamp(tmp_path / "nope.png") is False
|
||||
|
||||
@@ -421,3 +421,76 @@ class TestSessionConfiguration:
|
||||
def test_user_agent_and_accept_headers(self, helper):
|
||||
assert helper.session.headers["User-Agent"] == "LEDMatrix-Common/1.0"
|
||||
assert helper.session.headers["Accept"] == "image/*"
|
||||
|
||||
|
||||
class TestStalePlaceholderHandling:
|
||||
"""load_logo_with_download must not be fooled by a cached placeholder.
|
||||
|
||||
A placeholder wears the real logo's filename, so both the file cache and
|
||||
the in-memory cache can hold one and look like a hit.
|
||||
"""
|
||||
|
||||
def _placeholder(self, tmp_path, abbrev="COLL"):
|
||||
from src.logo_downloader import LogoDownloader
|
||||
assert LogoDownloader().create_placeholder_logo(abbrev, str(tmp_path))
|
||||
return tmp_path / f"{abbrev}.png"
|
||||
|
||||
def _make_stale(self, path):
|
||||
import time
|
||||
from PIL.PngImagePlugin import PngInfo
|
||||
from src.logo_downloader import PLACEHOLDER_MARKER, PLACEHOLDER_RETRY_SECONDS
|
||||
metadata = PngInfo()
|
||||
metadata.add_text(PLACEHOLDER_MARKER, str(time.time() - (PLACEHOLDER_RETRY_SECONDS + 60)))
|
||||
with Image.open(path) as img:
|
||||
img.copy().save(path, "PNG", pnginfo=metadata)
|
||||
|
||||
def test_fresh_placeholder_is_served_without_a_download(self, helper, tmp_path):
|
||||
path = self._placeholder(tmp_path)
|
||||
with patch.object(LogoHelper, "_download_logo") as download:
|
||||
assert helper.load_logo_with_download("COLL", path, "http://x/c.png") is not None
|
||||
download.assert_not_called()
|
||||
|
||||
def test_stale_placeholder_triggers_a_download(self, helper, tmp_path):
|
||||
path = self._placeholder(tmp_path)
|
||||
self._make_stale(path)
|
||||
with patch.object(LogoHelper, "_download_logo") as download:
|
||||
helper.load_logo_with_download("COLL", path, "http://x/c.png")
|
||||
download.assert_called_once()
|
||||
|
||||
def test_replacement_logo_is_not_masked_by_the_cached_placeholder(self, helper, tmp_path):
|
||||
"""The bug this guards: load_logo answers from cache before the disk.
|
||||
|
||||
Without invalidation the freshly downloaded logo would not appear until
|
||||
the process restarted.
|
||||
"""
|
||||
path = self._placeholder(tmp_path)
|
||||
first = helper.load_logo_with_download("COLL", path, "http://x/c.png")
|
||||
assert first is not None
|
||||
|
||||
self._make_stale(path)
|
||||
|
||||
def fake_download(_self, _url, file_path):
|
||||
Image.new("RGB", (500, 500), (7, 8, 9)).save(file_path, format="PNG")
|
||||
|
||||
with patch.object(LogoHelper, "_download_logo", fake_download):
|
||||
second = helper.load_logo_with_download("COLL", path, "http://x/c.png")
|
||||
|
||||
assert second is not None
|
||||
from src.logo_downloader import is_placeholder_logo
|
||||
assert is_placeholder_logo(path) is False
|
||||
assert second.getpixel((0, 0))[:3] == (7, 8, 9)
|
||||
|
||||
def test_failed_retry_restarts_the_back_off(self, helper, tmp_path):
|
||||
"""Otherwise a stale placeholder means a download attempt per call."""
|
||||
from src.logo_downloader import should_attempt_download
|
||||
path = self._placeholder(tmp_path)
|
||||
self._make_stale(path)
|
||||
assert should_attempt_download(path) is True
|
||||
|
||||
def boom(_self, _url, _file_path):
|
||||
raise OSError("network down")
|
||||
|
||||
with patch.object(LogoHelper, "_download_logo", boom):
|
||||
helper.load_logo_with_download("COLL", path, "http://x/c.png")
|
||||
|
||||
assert should_attempt_download(path) is False
|
||||
|
||||
Reference in New Issue
Block a user