From 99de401c03a26d20a6501c1439e187d48af9d187 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 09:26:25 -0400 Subject: [PATCH 1/2] fix(soccer-scoreboard): publish the league registry atomically on_config_change runs on the core's ConfigService-Watcher thread, not the render thread. It cleared self._league_registry and refilled it key by key while the display path iterates that same dict: _display_scroll_mode -> _get_enabled_leagues_for_mode -> for league_id, league_data in self._league_registry.items() So saving soccer config while a soccer frame was rendering could produce a frame with no leagues enabled, or raise "dictionary changed size during iteration" inside display(). _initialize_league_registry now builds into a local and publishes it with a single attribute rebind, which is atomic: a reader gets either the whole old registry or the whole new one. The .clear() in on_config_change is dropped, since clearing is exactly what made the gap visible. No lock needed and none added -- rebinding is already atomic, and a lock here would sit on the display path. Checked the rest of the repo for the same shape rather than fixing only the case I tripped over. Five plugins clear a dict inside on_config_change (afl-scoreboard, countdown, masters-tournament, nrl-scoreboard, soccer-scoreboard); soccer is the only one whose cleared dict is iterated elsewhere. The others are keyed-lookup caches, where a concurrent clear costs at most a cache miss. Also realigns version with versions[0], which had drifted (2.12.6 vs 2.12.2). The intermediate entries are still missing; they are not mine to invent. Tests: 15 existing pass; new test_league_registry_atomic_swap.py pins the mechanism (rebind not in-place, previously-published dict left intact, no self._league_registry[...] writes, no clear in on_config_change) and fails 4/7 against the pre-fix manager. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --- plugins/soccer-scoreboard/manager.py | 24 +++- plugins/soccer-scoreboard/manifest.json | 8 +- .../test_league_registry_atomic_swap.py | 112 ++++++++++++++++++ 3 files changed, 138 insertions(+), 6 deletions(-) create mode 100644 plugins/soccer-scoreboard/test_league_registry_atomic_swap.py diff --git a/plugins/soccer-scoreboard/manager.py b/plugins/soccer-scoreboard/manager.py index 0062e9ac..90e45788 100644 --- a/plugins/soccer-scoreboard/manager.py +++ b/plugins/soccer-scoreboard/manager.py @@ -823,7 +823,16 @@ def _initialize_league_registry(self) -> None: This design allows the display logic to iterate through leagues in priority order without hardcoding league names throughout the codebase. + + Built into a local and swapped in with a single assignment. The display + thread iterates ``self._league_registry`` (via + ``_get_enabled_leagues_for_mode``) while ``on_config_change`` rebuilds it + on the ConfigService-Watcher thread; mutating the live dict in place let + a frame observe it empty or half-populated, or raise "dictionary changed + size during iteration". Rebinding the attribute is atomic, so a reader + gets either the whole old registry or the whole new one. """ + registry: Dict[str, Dict[str, Any]] = {} # Add predefined leagues to registry for league_key in PREDEFINED_LEAGUE_KEYS: attr_tuple = PREDEFINED_LEAGUE_ATTR_MAP.get(league_key) @@ -831,7 +840,7 @@ def _initialize_league_registry(self) -> None: continue live_attr, recent_attr, upcoming_attr = attr_tuple - self._league_registry[league_key] = { + registry[league_key] = { 'enabled': self.league_enabled.get(league_key, False), 'priority': PREDEFINED_LEAGUE_PRIORITIES.get(league_key, 99), 'live_priority': self.league_live_priority.get(league_key, False), @@ -857,7 +866,7 @@ def _initialize_league_registry(self) -> None: recent_attr = f'custom_{safe_key}_recent' upcoming_attr = f'custom_{safe_key}_upcoming' - self._league_registry[league_code] = { + registry[league_code] = { 'enabled': self.league_enabled.get(league_code, False), 'priority': custom_priorities.get(league_code, 50), 'live_priority': self.league_live_priority.get(league_code, False), @@ -869,8 +878,11 @@ def _initialize_league_registry(self) -> None: } } + # Publish the finished registry in one atomic rebind (see docstring). + self._league_registry = registry + # Log registry state for debugging - enabled_leagues = [lid for lid, data in self._league_registry.items() if data['enabled']] + enabled_leagues = [lid for lid, data in registry.items() if data['enabled']] custom_count = len([lid for lid, data in self._league_registry.items() if data.get('is_custom', False)]) self.logger.info( f"League registry initialized: {len(self._league_registry)} league(s) registered " @@ -1343,8 +1355,10 @@ def on_config_change(self, new_config: Dict[str, Any]) -> None: thread.join(timeout=10.0) self._active_update_threads.clear() - # Clear stale runtime caches before rebuilding - self._league_registry.clear() + # Clear stale runtime caches before rebuilding. _league_registry is + # deliberately NOT cleared here: _initialize_league_registry() below + # rebuilds it into a local and swaps it in atomically, so the display + # thread never sees it empty. self._scroll_prepared.clear() self._scroll_active.clear() # Re-verify team codes against ESPN, so a corrected code is diff --git a/plugins/soccer-scoreboard/manifest.json b/plugins/soccer-scoreboard/manifest.json index 0af9ad62..57ac9597 100644 --- a/plugins/soccer-scoreboard/manifest.json +++ b/plugins/soccer-scoreboard/manifest.json @@ -1,7 +1,7 @@ { "id": "soccer-scoreboard", "name": "Soccer Scoreboard", - "version": "2.12.6", + "version": "2.12.7", "author": "ChuckBuilds", "description": "Live, recent, and upcoming soccer games across multiple leagues including Premier League, La Liga, Bundesliga, Serie A, Ligue 1, MLS, Liga Portugal, Champions League, Europa League, and FIFA World Cup", "category": "sports", @@ -26,6 +26,12 @@ "soccer_upcoming" ], "versions": [ + { + "version": "2.12.7", + "released": "2026-08-22", + "ledmatrix_min_version": "2.0.0", + "notes": "Rebuild the league registry into a local and publish it with one atomic rebind instead of clearing the live dict and refilling it key by key. on_config_change runs on the core's config-watcher thread while the display path iterates the same registry, so saving config during a soccer frame could render a screen with no leagues, or raise \"dictionary changed size during iteration\" inside display()." + }, { "version": "2.12.2", "released": "2026-08-19", diff --git a/plugins/soccer-scoreboard/test_league_registry_atomic_swap.py b/plugins/soccer-scoreboard/test_league_registry_atomic_swap.py new file mode 100644 index 00000000..df2b6495 --- /dev/null +++ b/plugins/soccer-scoreboard/test_league_registry_atomic_swap.py @@ -0,0 +1,112 @@ +#!/usr/bin/env python3 +""" +Regression test: rebuilding the league registry must not be visible mid-flight. + +``on_config_change`` runs on the core's ``ConfigService-Watcher`` thread, not +the render thread. It rebuilds the league registry, while the display path +iterates that same dict: + + _display_scroll_mode -> _get_enabled_leagues_for_mode + -> for league_id, league_data in self._league_registry.items() + +The rebuild used to clear the live dict and repopulate it key by key, so saving +config while a soccer frame was rendering could show a frame with no leagues, +or raise "dictionary changed size during iteration" inside display(). + +``_initialize_league_registry`` now builds into a local and publishes it with a +single attribute rebind, which is atomic. A reader sees either the whole old +registry or the whole new one. + +Run: /bin/python plugins/soccer-scoreboard/test_league_registry_atomic_swap.py +""" + +import sys +import types +from pathlib import Path + +plugin_dir = Path(__file__).parent +sys.path.insert(0, str(plugin_dir)) + + +def _stub_core_src(): + def mod(name, **attrs): + m = types.ModuleType(name) + for k, v in attrs.items(): + setattr(m, k, v) + sys.modules.setdefault(name, m) + return m + + mod("src") + mod("src.common") + mod("src.plugin_system") + mod("src.logo_downloader", LogoDownloader=object, download_missing_logo=lambda *a, **k: None) + mod("src.common.scroll_helper", ScrollHelper=object) + mod("src.plugin_system.base_plugin", BasePlugin=None, VegasDisplayMode=object) + mod("src.background_data_service", get_background_service=lambda *a, **k: None) + + +_stub_core_src() + +import logging # noqa: E402 +import inspect # noqa: E402 + +from manager import SoccerScoreboardPlugin # noqa: E402 + +results = [] + + +def check(case, passed): + results.append((case, passed)) + print(f" [{'pass' if passed else 'FAIL'}] {case}") + + +def make_plugin(): + p = SoccerScoreboardPlugin.__new__(SoccerScoreboardPlugin) + p.logger = logging.getLogger("test-soccer-registry-swap") + p.config = {"enabled": True, "custom_leagues": []} + p.display_manager = None + p.cache_manager = None + p.plugin_manager = None + p.league_enabled = {} + p.league_live_priority = {} + p._league_registry = {} + p.custom_league_map = {} + return p + + +plugin = make_plugin() +plugin._initialize_league_registry() +first = plugin._league_registry +check("registry is populated", len(first) > 0) + +# The old dict must survive the rebuild intact: a display thread that grabbed a +# reference before the swap keeps iterating a complete registry. +snapshot = plugin._league_registry +before_len = len(snapshot) +plugin._initialize_league_registry() + +check("rebuild rebinds rather than mutating in place", + plugin._league_registry is not snapshot) +check("the previously-published dict is left intact", + len(snapshot) == before_len) +check("new registry is complete", len(plugin._league_registry) == before_len) + +# Guard the mechanism itself, so a later refactor can't quietly reintroduce +# in-place mutation of the published dict. +src = inspect.getsource(SoccerScoreboardPlugin._initialize_league_registry) +check("no in-place writes to the published registry", + "self._league_registry[" not in src) +check("publishes with a single rebind", + src.count("self._league_registry = registry") == 1) + +occ_src = inspect.getsource(SoccerScoreboardPlugin.on_config_change) +check("on_config_change no longer clears the live registry", + "self._league_registry.clear()" not in occ_src) + +print() +failed = [case for case, passed in results if not passed] +print(f"{len(results) - len(failed)}/{len(results)} passed") +if failed: + for case in failed: + print(f" FAILED: {case}") + sys.exit(1) From 928dbbc2430b6ff0983d3139d3688fe9ae7a12f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 10:44:56 -0400 Subject: [PATCH 2/2] fix(soccer-scoreboard): keep one registry snapshot per selection Addresses the review finding. The atomic rebind stopped a reader seeing a half-built registry, but it left a second window open: readers that consult self._league_registry more than once can bind a different registry on each read. _get_enabled_leagues_for_mode iterates the registry, then reads it again in the sort key and once more in the debug line. A config save landing between the iteration and the sort publishes a replacement, and a custom league collected from the old registry need not exist in the new one -- so registry[lid] in the sort key raises KeyError, on the render thread. _get_league_manager_for_mode has the same shape: `if league_id not in self._league_registry` followed by self._league_registry[league_id]. A rebind between the two turns the membership test into a lie. Both now take one snapshot and use it throughout, so a whole selection is consistent with a single registry. _get_rankings_cache and _get_available_modes already read once and are unaffected. _initialize_league_registry's own logging now uses the local it just published rather than re-reading the attribute. Tests: two new cases. One replaces the registry from inside .items(), exactly when the selection is iterating, and asserts no KeyError and that the result reflects the registry the selection started from; it reproduces the KeyError when the sort key is reverted to re-reading the attribute. The other covers the manager lookup's test-then-index. 16 soccer tests pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --- plugins/soccer-scoreboard/manager.py | 27 ++++++++--- plugins/soccer-scoreboard/manifest.json | 2 +- .../test_league_registry_atomic_swap.py | 45 +++++++++++++++++++ 3 files changed, 66 insertions(+), 8 deletions(-) diff --git a/plugins/soccer-scoreboard/manager.py b/plugins/soccer-scoreboard/manager.py index 90e45788..0ba825cd 100644 --- a/plugins/soccer-scoreboard/manager.py +++ b/plugins/soccer-scoreboard/manager.py @@ -883,9 +883,9 @@ def _initialize_league_registry(self) -> None: # Log registry state for debugging enabled_leagues = [lid for lid, data in registry.items() if data['enabled']] - custom_count = len([lid for lid, data in self._league_registry.items() if data.get('is_custom', False)]) + custom_count = len([lid for lid, data in registry.items() if data.get('is_custom', False)]) self.logger.info( - f"League registry initialized: {len(self._league_registry)} league(s) registered " + f"League registry initialized: {len(registry)} league(s) registered " f"({custom_count} custom), {len(enabled_leagues)} enabled: " f"{[LEAGUE_NAMES.get(lid, lid) for lid in enabled_leagues]}" ) @@ -910,8 +910,17 @@ def _get_enabled_leagues_for_mode(self, mode_type: str) -> List[str]: """ enabled_leagues = [] + # One snapshot for the whole selection. _initialize_league_registry + # publishes a replacement registry by rebinding the attribute, so + # re-reading self._league_registry further down (the sort key, the debug + # line) could bind a NEWER registry than the one just iterated -- and a + # custom league collected from the old one may be absent from it, which + # is a KeyError in the sort. Reading once makes the whole selection + # consistent with a single registry. + registry = self._league_registry + # Iterate through all registered leagues - for league_id, league_data in self._league_registry.items(): + for league_id, league_data in registry.items(): # Check if league is enabled if not league_data.get('enabled', False): continue @@ -936,11 +945,11 @@ def _get_enabled_leagues_for_mode(self, mode_type: str) -> List[str]: enabled_leagues.append(league_id) # Sort by priority (lower number = higher priority) - enabled_leagues.sort(key=lambda lid: self._league_registry[lid].get('priority', 999)) + enabled_leagues.sort(key=lambda lid: registry[lid].get('priority', 999)) self.logger.debug( f"Enabled leagues for {mode_type} mode: {enabled_leagues} " - f"(priorities: {[self._league_registry[lid].get('priority') for lid in enabled_leagues]})" + f"(priorities: {[registry[lid].get('priority') for lid in enabled_leagues]})" ) return enabled_leagues @@ -1002,13 +1011,17 @@ def _get_league_manager_for_mode(self, league_id: str, mode_type: str): The manager is retrieved from the league registry, which is populated during initialization. If the league or mode doesn't exist, returns None. """ + # One snapshot: the membership test and the lookup must see the same + # registry, or a rebind between them turns the check into a KeyError. + registry = self._league_registry + # Check if league exists in registry - if league_id not in self._league_registry: + if league_id not in registry: self.logger.warning(f"League {league_id} not found in registry") return None # Get managers dict for this league - managers = self._league_registry[league_id].get('managers', {}) + managers = registry[league_id].get('managers', {}) # Get the manager for this mode type manager = managers.get(mode_type) diff --git a/plugins/soccer-scoreboard/manifest.json b/plugins/soccer-scoreboard/manifest.json index 57ac9597..fb0029c4 100644 --- a/plugins/soccer-scoreboard/manifest.json +++ b/plugins/soccer-scoreboard/manifest.json @@ -30,7 +30,7 @@ "version": "2.12.7", "released": "2026-08-22", "ledmatrix_min_version": "2.0.0", - "notes": "Rebuild the league registry into a local and publish it with one atomic rebind instead of clearing the live dict and refilling it key by key. on_config_change runs on the core's config-watcher thread while the display path iterates the same registry, so saving config during a soccer frame could render a screen with no leagues, or raise \"dictionary changed size during iteration\" inside display()." + "notes": "Rebuild the league registry into a local and publish it with one atomic rebind instead of clearing the live dict and refilling it key by key. on_config_change runs on the core's config-watcher thread while the display path iterates the same registry, so saving config during a soccer frame could render a screen with no leagues, or raise \"dictionary changed size during iteration\" inside display(). Readers that consult the registry more than once (the league selection's iterate-then-sort, and the manager lookup's membership-test-then-index) now take a single snapshot, so a replacement landing mid-selection cannot turn a league collected from the old registry into a KeyError against the new one." }, { "version": "2.12.2", diff --git a/plugins/soccer-scoreboard/test_league_registry_atomic_swap.py b/plugins/soccer-scoreboard/test_league_registry_atomic_swap.py index df2b6495..5e15c3ed 100644 --- a/plugins/soccer-scoreboard/test_league_registry_atomic_swap.py +++ b/plugins/soccer-scoreboard/test_league_registry_atomic_swap.py @@ -103,6 +103,51 @@ def make_plugin(): check("on_config_change no longer clears the live registry", "self._league_registry.clear()" not in occ_src) +# --- a reader must survive a replacement landing mid-selection ------------ +# _get_enabled_leagues_for_mode iterates the registry, then sorts and logs +# using it again. If those later reads re-fetched self._league_registry, a +# config save that drops a custom league between the iteration and the sort +# would raise KeyError on the render thread. +swapped = make_plugin() +swapped._league_registry = { + "eng.1": {"enabled": True, "priority": 1, "is_custom": False, "managers": {}}, + "cus.1": {"enabled": True, "priority": 2, "is_custom": True, "managers": {}}, +} +swapped._get_league_config = lambda lid, data: {} + +original_items = swapped._league_registry.items + + +class _SwapOnIterate(dict): + """Rebinds the plugin's registry the moment the selection iterates it, + standing in for on_config_change landing on the watcher thread.""" + + def items(self): + # The replacement no longer has the custom league. + swapped._league_registry = { + "eng.1": {"enabled": True, "priority": 1, "is_custom": False, "managers": {}}, + } + return original_items() + + +swapped._league_registry = _SwapOnIterate(swapped._league_registry) +try: + selected = swapped._get_enabled_leagues_for_mode("live") + raised = None +except KeyError as exc: + selected, raised = None, exc + +check("a registry replacement mid-selection does not raise KeyError", raised is None) +check("the selection reflects the registry it started from", + selected is not None and "cus.1" in selected) + +# The membership test and lookup in _get_league_manager_for_mode must also +# agree with each other. +mgr_plugin = make_plugin() +mgr_plugin._league_registry = {"eng.1": {"enabled": True, "managers": {"live": "M"}}} +check("manager lookup returns from the same snapshot it tested", + mgr_plugin._get_league_manager_for_mode("eng.1", "live") == "M") + print() failed = [case for case, passed in results if not passed] print(f"{len(results) - len(failed)}/{len(results)} passed")