From c87c73d0a8c24b286aa130963db79b47a4763b46 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Wed, 21 Oct 2026 05:05:46 +0200 Subject: [PATCH] fix: replace the system icon cache whole The cache was truncated and dumped in place. A concurrent build read half a file, took it for empty, and an offline run then wrote {} over every recorded icon. The file is now replaced through a scratch file, merged with what another run recorded meanwhile, and left alone when nothing new was learned. --- scripts/siterender.py | 60 ++++++++++++++++++++-------- tests/test_site_render.py | 84 +++++++++++++++++++++++++++++++++++++++ wiki/testing-guide.md | 2 +- 3 files changed, 129 insertions(+), 17 deletions(-) create mode 100644 tests/test_site_render.py diff --git a/scripts/siterender.py b/scripts/siterender.py index 33f3afc6..1b17b85b 100644 --- a/scripts/siterender.py +++ b/scripts/siterender.py @@ -2,12 +2,17 @@ from __future__ import annotations -import re - -from pathlib import Path -from concurrent.futures import ThreadPoolExecutor import json +import re +import sys +import urllib.error +import urllib.parse import urllib.request +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path + +from artifacts import write_text_atomic + WIKI_SRC_DIR = "wiki" # manually maintained wiki sources SYSTEM_ICON_BASE = "https://raw.githubusercontent.com/libretro/retroarch-assets/master/xmb/systematic/png" ICON_CACHE_PATH = Path(".cache") / "system_icons.json" @@ -67,6 +72,27 @@ def _icon_name(manufacturer: str, console_name: str) -> str: def _icon_url(icon_name: str) -> str: return f"{SYSTEM_ICON_BASE}/{urllib.parse.quote(icon_name)}.png" +def _read_icon_cache() -> dict[str, bool]: + """The recorded icon checks, empty when none were recorded. + + An unreadable file is reported, not taken for an empty cache in silence. + """ + try: + text = ICON_CACHE_PATH.read_text(encoding="utf-8") + except FileNotFoundError: + return {} + except OSError as exc: + print(f" WARNING: cannot read {ICON_CACHE_PATH}: {exc}", file=sys.stderr) + return {} + try: + data = json.loads(text) + except json.JSONDecodeError as exc: + print(f" WARNING: {ICON_CACHE_PATH} is not valid JSON ({exc}), " + "icons are checked again", file=sys.stderr) + return {} + return {str(k): bool(v) for k, v in data.items()} if isinstance(data, dict) else {} + + def prime_system_icons(names: set[str]) -> None: """Record which system icons upstream actually serves. @@ -74,14 +100,7 @@ def prime_system_icons(names: set[str]) -> None: cannot be checked stay unavailable: the site never links an image it has not seen answer. """ - cached: dict[str, bool] = {} - if ICON_CACHE_PATH.exists(): - try: - with open(ICON_CACHE_PATH) as f: - cached = json.load(f) - except (json.JSONDecodeError, OSError): - cached = {} - + cached = _read_icon_cache() unknown = sorted(n for n in names if n not in cached) if unknown: def check(name: str) -> tuple[str, bool | None]: @@ -100,13 +119,22 @@ def prime_system_icons(names: set[str]) -> None: return name, None print(f"Checking {len(unknown)} system icons...") + learned: dict[str, bool] = {} with ThreadPoolExecutor(max_workers=8) as pool: for name, ok in pool.map(check, unknown): if ok is not None: - cached[name] = ok - ICON_CACHE_PATH.parent.mkdir(parents=True, exist_ok=True) - with open(ICON_CACHE_PATH, "w") as f: - json.dump(cached, f, indent=2, sort_keys=True) + learned[name] = ok + cached.update(learned) + if learned: + # Another run may have recorded names meanwhile: merge with what + # the file holds now, and replace it whole so a reader never sees + # a half-written cache. + merged = {**_read_icon_cache(), **learned} + ICON_CACHE_PATH.parent.mkdir(parents=True, exist_ok=True) + write_text_atomic( + str(ICON_CACHE_PATH), + json.dumps(merged, indent=2, sort_keys=True) + "\n", + ) _icon_available.update(cached) missing = sum(1 for n in names if not cached.get(n)) diff --git a/tests/test_site_render.py b/tests/test_site_render.py new file mode 100644 index 00000000..dd7aab80 --- /dev/null +++ b/tests/test_site_render.py @@ -0,0 +1,84 @@ +"""Renderings shared by the site pages, and the icon cache they read. + +No network: icon checks are answered by a stub. +""" + +from __future__ import annotations + +import json +import sys +import tempfile +import unittest +import urllib.error +from pathlib import Path +from unittest import mock + +REPO_ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(REPO_ROOT / "scripts")) + +import siterender # noqa: E402 + + +class _Served: + status = 200 + + def __enter__(self) -> "_Served": + return self + + def __exit__(self, *exc: object) -> None: + return None + + +class IconCacheIsReplacedWhole(unittest.TestCase): + """The cache was truncated then dumped in place: a concurrent reader saw + half a file, took it for empty, and an offline run then wrote ``{}`` + over every recorded icon.""" + + def setUp(self) -> None: + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.path = Path(tmp.name) / "system_icons.json" + patcher = mock.patch.object(siterender, "ICON_CACHE_PATH", self.path) + patcher.start() + self.addCleanup(patcher.stop) + saved = dict(siterender._icon_available) + self.addCleanup(lambda: (siterender._icon_available.clear(), + siterender._icon_available.update(saved))) + + def test_an_offline_run_leaves_an_unreadable_cache_untouched(self): + self.path.write_text('{"Sega - Mega Drive": tr') + offline = mock.patch.object( + siterender.urllib.request, "urlopen", + side_effect=urllib.error.URLError("offline")) + with offline, mock.patch("sys.stderr"), mock.patch("builtins.print"): + siterender.prime_system_icons({"Sega - Mega Drive"}) + self.assertEqual(self.path.read_text(), '{"Sega - Mega Drive": tr') + + def test_names_another_run_recorded_meanwhile_are_kept(self): + self.path.write_text(json.dumps({"A - One": True})) + + def answer(*_args: object, **_kwargs: object) -> _Served: + # A second build records its own check while this one runs. + self.path.write_text(json.dumps({"A - One": True, "C - Three": False})) + return _Served() + + with mock.patch.object(siterender.urllib.request, "urlopen", side_effect=answer), \ + mock.patch("builtins.print"): + siterender.prime_system_icons({"A - One", "B - Two"}) + self.assertEqual(json.loads(self.path.read_text()), + {"A - One": True, "B - Two": True, "C - Three": False}) + self.assertEqual(sorted(p.name for p in self.path.parent.iterdir()), + ["system_icons.json"]) + + def test_the_write_goes_through_a_scratch_file(self): + with mock.patch.object(siterender.urllib.request, "urlopen", + return_value=_Served()), \ + mock.patch.object(siterender, "write_text_atomic", + wraps=siterender.write_text_atomic) as atomic, \ + mock.patch("builtins.print"): + siterender.prime_system_icons({"B - Two"}) + atomic.assert_called_once() + + +if __name__ == "__main__": + unittest.main() diff --git a/wiki/testing-guide.md b/wiki/testing-guide.md index bf4c3c1d..e7b7630a 100644 --- a/wiki/testing-guide.md +++ b/wiki/testing-guide.md @@ -46,7 +46,7 @@ Every module in `tests/`, by the area it guards. | Installer and downloads | loopback server | `test_check_release`, `test_download`, `test_install`, `test_install_cores`, `test_install_experience`, `test_install_selection` | | Truth and native export | synthetic + cached originals | `test_export_counts`, `test_export_native_layer`, `test_export_report`, `test_exporters`, `test_truth_diff`, `test_truth_layout` | | Scrapers, upstream and provenance | inline sources, synthetic | `test_check_freshness`, `test_dedup`, `test_fbneo_parser`, `test_find_pinned_revision`, `test_hash_merge`, `test_launchers`, `test_launchers_emudeck`, `test_launchers_recalbox`, `test_launchers_retrobat`, `test_launchers_retrodeck`, `test_launchers_retropie`, `test_launchers_rocknix`, `test_mame_devices`, `test_mame_parser`, `test_mame_ref_audit`, `test_profile_sync`, `test_provenance`, `test_refresh_data_dirs`, `test_refresh_failures`, `test_refresh_stale`, `test_retrodeck_scraper_errors`, `test_romset_recipes`, `test_scraper_contract`, `test_upstream`, `test_upstream_index` | -| Site, docs and repository contracts | rendered pages, sources | `test_contributing_text`, `test_data_dir_refs`, `test_dataclass_defaults`, `test_optional_dependencies`, `test_prose_style`, `test_site_availability`, `test_site_cores`, `test_site_exports`, `test_site_platform_hashes`, `test_site_system_links`, `test_site_validation`, `test_site_write`, `test_validate_pr`, `test_validate_pr_hashes`, `test_workflow_paths` | +| Site, docs and repository contracts | rendered pages, sources | `test_contributing_text`, `test_data_dir_refs`, `test_dataclass_defaults`, `test_optional_dependencies`, `test_prose_style`, `test_site_availability`, `test_site_cores`, `test_site_exports`, `test_site_platform_hashes`, `test_site_render`, `test_site_system_links`, `test_site_validation`, `test_site_write`, `test_validate_pr`, `test_validate_pr_hashes`, `test_workflow_paths` | ## Test architecture