From 19d7b42b3578005e1865548daaaf643994b8674b Mon Sep 17 00:00:00 2001 From: Chuck Date: Mon, 9 Feb 2026 19:40:40 -0500 Subject: [PATCH 1/6] fix(plugins): prevent KeyError race condition in module cleanup When multiple plugins have modules with the same name (e.g., background_data_service.py), the _clear_conflicting_modules function could raise a KeyError if a module was removed between iteration and deletion. This race condition caused plugin loading failures with errors like: "Unexpected error loading plugin: 'background_data_service'" Changes: - Use sys.modules.pop(mod_name, None) instead of del sys.modules[mod_name] to safely handle already-removed modules - Apply same fix to plugin unload in plugin_manager.py for consistency - Fix typo in sports.py: rankself._team_rankings_cacheings -> self._team_rankings_cache Co-Authored-By: Claude Opus 4.5 --- src/base_classes/sports.py | 2 +- src/plugin_system/plugin_loader.py | 4 ++-- src/plugin_system/plugin_manager.py | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/base_classes/sports.py b/src/base_classes/sports.py index 56564cd3e..5dba8cf7f 100644 --- a/src/base_classes/sports.py +++ b/src/base_classes/sports.py @@ -890,7 +890,7 @@ def _draw_scorebug_layout(self, game: Dict, force_clear: bool = False) -> None: away_text = '' elif self.show_ranking: # Show ranking only if available - away_rank = rankself._team_rankings_cacheings.get(away_abbr, 0) + away_rank = self._team_rankings_cache.get(away_abbr, 0) if away_rank > 0: away_text = f"#{away_rank}" else: diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index a535ce36a..e5e839318 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -230,9 +230,9 @@ def _clear_conflicting_modules(self, plugin_dir: Path, plugin_id: str) -> None: mod_name, mod_file, plugin_id ) - # Remove conflicting modules + # Remove conflicting modules (use pop to avoid KeyError if already removed) for mod_name in modules_to_remove: - del sys.modules[mod_name] + sys.modules.pop(mod_name, None) def load_module( self, diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index ba09c1834..118494f93 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -417,8 +417,7 @@ def unload_plugin(self, plugin_id: str) -> bool: # Remove module from sys.modules if present module_name = f"plugin_{plugin_id.replace('-', '_')}" - if module_name in sys.modules: - del sys.modules[module_name] + sys.modules.pop(module_name, None) # Remove from plugin_modules self.plugin_modules.pop(plugin_id, None) From 74731ae49f8374d18da3e830b401620f09c0dae3 Mon Sep 17 00:00:00 2001 From: Chuck Date: Tue, 10 Feb 2026 22:02:07 -0500 Subject: [PATCH 2/6] fix(plugins): namespace-isolate plugin modules to prevent parallel loading collisions Multiple sport plugins share identically-named Python files (scroll_display.py, game_renderer.py, sports.py, etc.). When loaded in parallel via ThreadPoolExecutor, bare module names collide in sys.modules causing KeyError crashes. Replace _clear_conflicting_modules with _namespace_plugin_modules: after exec_module loads a plugin, its bare-name sub-modules are moved to namespaced keys (e.g. _plg_basketball_scoreboard_scroll_display) so they cannot collide. A threading lock serializes the exec_module window where bare names temporarily exist. Also updates unload_plugin to clean up namespaced sub-modules from sys.modules. Co-Authored-By: Claude Opus 4.6 --- src/plugin_system/plugin_loader.py | 142 ++++++++++++++++++---------- src/plugin_system/plugin_manager.py | 10 +- 2 files changed, 99 insertions(+), 53 deletions(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index e5e839318..7b37e3d3f 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -10,6 +10,7 @@ import importlib.util import sys import subprocess +import threading from pathlib import Path from typing import Dict, Any, Optional, Tuple, Type import logging @@ -35,6 +36,12 @@ def __init__(self, logger: Optional[logging.Logger] = None) -> None: self.logger = logger or get_logger(__name__) self._loaded_modules: Dict[str, Any] = {} self._plugin_module_registry: Dict[str, set] = {} # Maps plugin_id to set of module names + # Lock to serialize module loading when plugins share module names + # (e.g., scroll_display.py, game_renderer.py across sport plugins). + # Without this, parallel plugin loading via ThreadPoolExecutor can + # cause one thread's _clear_conflicting_modules to remove a module + # from sys.modules while another thread is actively importing it. + self._module_load_lock = threading.Lock() def find_plugin_directory( self, @@ -190,49 +197,68 @@ def install_dependencies( self.logger.error("Unexpected error installing dependencies for %s: %s", plugin_id, e, exc_info=True) return False - def _clear_conflicting_modules(self, plugin_dir: Path, plugin_id: str) -> None: + def _namespace_plugin_modules( + self, plugin_id: str, plugin_dir: Path, before_keys: set + ) -> None: """ - Clear cached modules from sys.modules that could conflict with the plugin being loaded. + Move bare-name plugin modules to namespaced keys in sys.modules. - When multiple plugins have modules with the same name (e.g., data_fetcher.py), - Python's module cache can cause the wrong module to be imported. This method - removes cached modules from other plugin directories to ensure the correct - module is loaded. + After exec_module loads a plugin's entry point, Python will have added + the plugin's local modules (scroll_display, game_renderer, …) to + sys.modules under their bare names. This method renames them to + ``_plg__`` so they cannot collide with identically- + named modules from other plugins. + + The plugin code keeps working because ``from scroll_display import X`` + binds ``X`` to the class *object*, not to the sys.modules entry. Args: - plugin_dir: The directory of the plugin being loaded - plugin_id: The plugin identifier + plugin_id: Plugin identifier + plugin_dir: Plugin directory path + before_keys: Snapshot of sys.modules keys taken *before* exec_module """ plugin_dir_str = str(plugin_dir) + safe_id = plugin_id.replace("-", "_") + namespaced_names: set = set() - # Find modules to remove: those from other plugin directories - # that could conflict with modules in the current plugin - modules_to_remove = [] + after_keys = set(sys.modules.keys()) + new_keys = after_keys - before_keys + + for mod_name in new_keys: + # Only touch bare names (no dots) — dotted names are packages or + # stdlib/site-packages modules that should be left alone. + if "." in mod_name: + continue - for mod_name, mod in list(sys.modules.items()): + mod = sys.modules.get(mod_name) if mod is None: continue - # Skip standard library and site-packages modules - mod_file = getattr(mod, '__file__', None) + mod_file = getattr(mod, "__file__", None) if mod_file is None: continue - # Check if this module is from a different plugin directory - # We look for modules that: - # 1. Have simple names (no dots) - these are the ones that conflict - # 2. Are loaded from a plugin-repos directory but not the current plugin - if '.' not in mod_name and 'plugin-repos' in mod_file: - if plugin_dir_str not in mod_file: - modules_to_remove.append(mod_name) - self.logger.debug( - "Clearing cached module '%s' from %s to avoid conflict with plugin %s", - mod_name, mod_file, plugin_id - ) - - # Remove conflicting modules (use pop to avoid KeyError if already removed) - for mod_name in modules_to_remove: + # Only rename modules whose source lives inside this plugin dir + if plugin_dir_str not in mod_file: + continue + + namespaced = f"_plg_{safe_id}_{mod_name}" + sys.modules[namespaced] = mod sys.modules.pop(mod_name, None) + namespaced_names.add(namespaced) + self.logger.debug( + "Namespace-isolated module '%s' -> '%s' for plugin %s", + mod_name, namespaced, plugin_id, + ) + + # Track for cleanup during unload + self._plugin_module_registry[plugin_id] = namespaced_names + + if namespaced_names: + self.logger.info( + "Namespace-isolated %d module(s) for plugin %s", + len(namespaced_names), plugin_id, + ) def load_module( self, @@ -243,6 +269,15 @@ def load_module( """ Load a plugin module from file. + Module loading is serialized via _module_load_lock because plugins are + loaded in parallel (ThreadPoolExecutor) and multiple sport plugins + share identically-named local modules (scroll_display.py, + game_renderer.py, sports.py, etc.). + + After loading, bare-name modules from the plugin directory are moved + to namespaced keys in sys.modules (e.g. ``_plg_basketball_scoreboard_scroll_display``) + so they cannot collide with other plugins. + Args: plugin_id: Plugin identifier plugin_dir: Plugin directory path @@ -257,35 +292,40 @@ def load_module( self.logger.error(error_msg) raise PluginError(error_msg, plugin_id=plugin_id, context={'entry_file': str(entry_file)}) - # Clear any conflicting modules from other plugins before loading - self._clear_conflicting_modules(plugin_dir, plugin_id) + with self._module_load_lock: + # Snapshot sys.modules so we can detect new bare-name entries + before_keys = set(sys.modules.keys()) - # Add plugin directory to sys.path if not already there - plugin_dir_str = str(plugin_dir) - if plugin_dir_str not in sys.path: - sys.path.insert(0, plugin_dir_str) - self.logger.debug("Added plugin directory to sys.path: %s", plugin_dir_str) + # Add plugin directory to sys.path if not already there + plugin_dir_str = str(plugin_dir) + if plugin_dir_str not in sys.path: + sys.path.insert(0, plugin_dir_str) + self.logger.debug("Added plugin directory to sys.path: %s", plugin_dir_str) - # Import the plugin module - module_name = f"plugin_{plugin_id.replace('-', '_')}" + # Import the plugin module + module_name = f"plugin_{plugin_id.replace('-', '_')}" - # Check if already loaded - if module_name in sys.modules: - self.logger.debug("Module %s already loaded, reusing", module_name) - return sys.modules[module_name] + # Check if already loaded + if module_name in sys.modules: + self.logger.debug("Module %s already loaded, reusing", module_name) + return sys.modules[module_name] - spec = importlib.util.spec_from_file_location(module_name, entry_file) - if spec is None or spec.loader is None: - error_msg = f"Could not create module spec for {entry_file}" - self.logger.error(error_msg) - raise PluginError(error_msg, plugin_id=plugin_id, context={'entry_file': str(entry_file)}) + spec = importlib.util.spec_from_file_location(module_name, entry_file) + if spec is None or spec.loader is None: + error_msg = f"Could not create module spec for {entry_file}" + self.logger.error(error_msg) + raise PluginError(error_msg, plugin_id=plugin_id, context={'entry_file': str(entry_file)}) + + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + spec.loader.exec_module(module) - module = importlib.util.module_from_spec(spec) - sys.modules[module_name] = module - spec.loader.exec_module(module) + # Move bare-name plugin modules to namespaced keys so they + # cannot collide with identically-named modules from other plugins + self._namespace_plugin_modules(plugin_id, plugin_dir, before_keys) - self._loaded_modules[plugin_id] = module - self.logger.debug("Loaded module %s for plugin %s", module_name, plugin_id) + self._loaded_modules[plugin_id] = module + self.logger.debug("Loaded module %s for plugin %s", module_name, plugin_id) return module diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 118494f93..ce9933945 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -415,10 +415,16 @@ def unload_plugin(self, plugin_id: str) -> bool: if plugin_id in self.plugin_last_update: del self.plugin_last_update[plugin_id] - # Remove module from sys.modules if present + # Remove main module from sys.modules if present module_name = f"plugin_{plugin_id.replace('-', '_')}" sys.modules.pop(module_name, None) - + + # Remove namespaced sub-modules tracked by the plugin loader + # (e.g. _plg_basketball_scoreboard_scroll_display) + if hasattr(self, 'plugin_loader') and hasattr(self.plugin_loader, '_plugin_module_registry'): + for ns_name in self.plugin_loader._plugin_module_registry.pop(plugin_id, set()): + sys.modules.pop(ns_name, None) + # Remove from plugin_modules self.plugin_modules.pop(plugin_id, None) From f151d327601074f2f979d65e929e7c0fb05ad8ef Mon Sep 17 00:00:00 2001 From: Chuck Date: Tue, 10 Feb 2026 22:36:03 -0500 Subject: [PATCH 3/6] fix(plugins): address review feedback on namespace isolation - Fix main module accidentally renamed: move before_keys snapshot to after sys.modules[module_name] insertion so the main entry is excluded from namespace renaming and error cleanup - Use Path.is_relative_to() instead of substring matching for plugin directory containment checks to avoid false-matches on overlapping directory names - Add try/except around exec_module to clean up partially-initialized modules on failure, preventing leaked bare-name entries - Add public unregister_plugin_modules() method on PluginLoader so PluginManager doesn't reach into private attributes during unload - Update stale comment referencing removed _clear_conflicting_modules Co-Authored-By: Claude Opus 4.6 --- src/plugin_system/plugin_loader.py | 60 +++++++++++++++++++++++------ src/plugin_system/plugin_manager.py | 7 +--- 2 files changed, 51 insertions(+), 16 deletions(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 7b37e3d3f..c04f5b314 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -38,9 +38,11 @@ def __init__(self, logger: Optional[logging.Logger] = None) -> None: self._plugin_module_registry: Dict[str, set] = {} # Maps plugin_id to set of module names # Lock to serialize module loading when plugins share module names # (e.g., scroll_display.py, game_renderer.py across sport plugins). - # Without this, parallel plugin loading via ThreadPoolExecutor can - # cause one thread's _clear_conflicting_modules to remove a module - # from sys.modules while another thread is actively importing it. + # During exec_module, bare-name sub-modules temporarily appear in + # sys.modules; the lock prevents concurrent plugins from seeing each + # other's entries. After exec_module, _namespace_plugin_modules + # moves those bare names to namespaced keys (e.g. + # _plg_basketball_scoreboard_scroll_display) so they never collide. self._module_load_lock = threading.Lock() def find_plugin_directory( @@ -239,7 +241,10 @@ def _namespace_plugin_modules( continue # Only rename modules whose source lives inside this plugin dir - if plugin_dir_str not in mod_file: + try: + if not Path(mod_file).resolve().is_relative_to(plugin_dir.resolve()): + continue + except (ValueError, TypeError): continue namespaced = f"_plg_{safe_id}_{mod_name}" @@ -260,6 +265,16 @@ def _namespace_plugin_modules( len(namespaced_names), plugin_id, ) + def unregister_plugin_modules(self, plugin_id: str) -> None: + """Remove namespaced sub-modules and cached module for a plugin from sys.modules. + + Called by PluginManager during unload to clean up all module entries + that were created when the plugin was loaded. + """ + for ns_name in self._plugin_module_registry.pop(plugin_id, set()): + sys.modules.pop(ns_name, None) + self._loaded_modules.pop(plugin_id, None) + def load_module( self, plugin_id: str, @@ -293,9 +308,6 @@ def load_module( raise PluginError(error_msg, plugin_id=plugin_id, context={'entry_file': str(entry_file)}) with self._module_load_lock: - # Snapshot sys.modules so we can detect new bare-name entries - before_keys = set(sys.modules.keys()) - # Add plugin directory to sys.path if not already there plugin_dir_str = str(plugin_dir) if plugin_dir_str not in sys.path: @@ -318,11 +330,37 @@ def load_module( module = importlib.util.module_from_spec(spec) sys.modules[module_name] = module - spec.loader.exec_module(module) - # Move bare-name plugin modules to namespaced keys so they - # cannot collide with identically-named modules from other plugins - self._namespace_plugin_modules(plugin_id, plugin_dir, before_keys) + # Snapshot AFTER inserting the main module so that + # _namespace_plugin_modules and error cleanup only target + # sub-modules, not the main module entry itself. + before_keys = set(sys.modules.keys()) + try: + spec.loader.exec_module(module) + + # Move bare-name plugin modules to namespaced keys so they + # cannot collide with identically-named modules from other plugins + self._namespace_plugin_modules(plugin_id, plugin_dir, before_keys) + except Exception: + # Clean up the partially-initialized main module and any + # bare-name sub-modules that were added during exec_module + # so they don't leak into subsequent plugin loads. + sys.modules.pop(module_name, None) + for key in set(sys.modules.keys()) - before_keys: + if "." in key: + continue + mod = sys.modules.get(key) + if mod is None: + continue + mod_file = getattr(mod, "__file__", None) + if not mod_file: + continue + try: + if Path(mod_file).resolve().is_relative_to(plugin_dir.resolve()): + sys.modules.pop(key, None) + except (ValueError, TypeError): + continue + raise self._loaded_modules[plugin_id] = module self.logger.debug("Loaded module %s for plugin %s", module_name, plugin_id) diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index ce9933945..b746164af 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -419,11 +419,8 @@ def unload_plugin(self, plugin_id: str) -> bool: module_name = f"plugin_{plugin_id.replace('-', '_')}" sys.modules.pop(module_name, None) - # Remove namespaced sub-modules tracked by the plugin loader - # (e.g. _plg_basketball_scoreboard_scroll_display) - if hasattr(self, 'plugin_loader') and hasattr(self.plugin_loader, '_plugin_module_registry'): - for ns_name in self.plugin_loader._plugin_module_registry.pop(plugin_id, set()): - sys.modules.pop(ns_name, None) + # Delegate sub-module and cached-module cleanup to the loader + self.plugin_loader.unregister_plugin_modules(plugin_id) # Remove from plugin_modules self.plugin_modules.pop(plugin_id, None) From fb17f5db9c36dc45545e7ed582b8d427fb3f21b4 Mon Sep 17 00:00:00 2001 From: Chuck Date: Tue, 10 Feb 2026 22:42:06 -0500 Subject: [PATCH 4/6] fix(plugins): remove unused plugin_dir_str variable Leftover from the old substring containment check, now replaced by Path.is_relative_to(). Co-Authored-By: Claude Opus 4.6 --- src/plugin_system/plugin_loader.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index c04f5b314..76071495b 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -219,7 +219,6 @@ def _namespace_plugin_modules( plugin_dir: Plugin directory path before_keys: Snapshot of sys.modules keys taken *before* exec_module """ - plugin_dir_str = str(plugin_dir) safe_id = plugin_id.replace("-", "_") namespaced_names: set = set() From 5bbb62b0994e015e6a673ae9caf6328b862af21b Mon Sep 17 00:00:00 2001 From: Chuck Date: Tue, 10 Feb 2026 22:43:15 -0500 Subject: [PATCH 5/6] fix(plugins): extract shared helper for bare-module filtering Hoist plugin_dir.resolve() out of loops and deduplicate the bare-module filtering logic between _namespace_plugin_modules and the error cleanup block into _iter_plugin_bare_modules(). Co-Authored-By: Claude Opus 4.6 --- src/plugin_system/plugin_loader.py | 70 ++++++++++++++---------------- 1 file changed, 32 insertions(+), 38 deletions(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 76071495b..31498cecc 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -199,6 +199,35 @@ def install_dependencies( self.logger.error("Unexpected error installing dependencies for %s: %s", plugin_id, e, exc_info=True) return False + @staticmethod + def _iter_plugin_bare_modules( + plugin_dir: Path, before_keys: set + ) -> list: + """Return bare-name modules from plugin_dir added after before_keys. + + Returns a list of (mod_name, module) tuples for modules that: + - Were added to sys.modules after before_keys snapshot + - Have bare names (no dots) + - Have a ``__file__`` inside plugin_dir + """ + resolved_dir = plugin_dir.resolve() + result = [] + for key in set(sys.modules.keys()) - before_keys: + if "." in key: + continue + mod = sys.modules.get(key) + if mod is None: + continue + mod_file = getattr(mod, "__file__", None) + if not mod_file: + continue + try: + if Path(mod_file).resolve().is_relative_to(resolved_dir): + result.append((key, mod)) + except (ValueError, TypeError): + continue + return result + def _namespace_plugin_modules( self, plugin_id: str, plugin_dir: Path, before_keys: set ) -> None: @@ -222,30 +251,7 @@ def _namespace_plugin_modules( safe_id = plugin_id.replace("-", "_") namespaced_names: set = set() - after_keys = set(sys.modules.keys()) - new_keys = after_keys - before_keys - - for mod_name in new_keys: - # Only touch bare names (no dots) — dotted names are packages or - # stdlib/site-packages modules that should be left alone. - if "." in mod_name: - continue - - mod = sys.modules.get(mod_name) - if mod is None: - continue - - mod_file = getattr(mod, "__file__", None) - if mod_file is None: - continue - - # Only rename modules whose source lives inside this plugin dir - try: - if not Path(mod_file).resolve().is_relative_to(plugin_dir.resolve()): - continue - except (ValueError, TypeError): - continue - + for mod_name, mod in self._iter_plugin_bare_modules(plugin_dir, before_keys): namespaced = f"_plg_{safe_id}_{mod_name}" sys.modules[namespaced] = mod sys.modules.pop(mod_name, None) @@ -345,20 +351,8 @@ def load_module( # bare-name sub-modules that were added during exec_module # so they don't leak into subsequent plugin loads. sys.modules.pop(module_name, None) - for key in set(sys.modules.keys()) - before_keys: - if "." in key: - continue - mod = sys.modules.get(key) - if mod is None: - continue - mod_file = getattr(mod, "__file__", None) - if not mod_file: - continue - try: - if Path(mod_file).resolve().is_relative_to(plugin_dir.resolve()): - sys.modules.pop(key, None) - except (ValueError, TypeError): - continue + for key, _ in self._iter_plugin_bare_modules(plugin_dir, before_keys): + sys.modules.pop(key, None) raise self._loaded_modules[plugin_id] = module From 321bafd192f4faa71d30874dea5c152359470edb Mon Sep 17 00:00:00 2001 From: Chuck Date: Tue, 10 Feb 2026 22:44:19 -0500 Subject: [PATCH 6/6] fix(plugins): keep bare-name alias to prevent lazy import duplication Stop removing bare module names from sys.modules after namespacing. Removing them caused lazy intra-plugin imports (deferred imports inside methods) to re-import from disk, creating a second inconsistent module copy. Keeping both the bare and namespaced entries pointing to the same object avoids this. The next plugin's exec_module naturally overwrites the bare entry with its own version. Co-Authored-By: Claude Opus 4.6 --- src/plugin_system/plugin_loader.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index 31498cecc..5bde651af 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -254,7 +254,12 @@ def _namespace_plugin_modules( for mod_name, mod in self._iter_plugin_bare_modules(plugin_dir, before_keys): namespaced = f"_plg_{safe_id}_{mod_name}" sys.modules[namespaced] = mod - sys.modules.pop(mod_name, None) + # Keep sys.modules[mod_name] as an alias to the same object. + # Removing it would cause lazy intra-plugin imports (e.g. a + # deferred ``import scroll_display`` inside a method) to + # re-import from disk and create a second, inconsistent copy + # of the module. The next plugin's exec_module will naturally + # overwrite the bare entry with its own version. namespaced_names.add(namespaced) self.logger.debug( "Namespace-isolated module '%s' -> '%s' for plugin %s",