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

@@ -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))

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()

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