fix(plugins): sub-package reload, symlinked dev plugins, BaseException in update(), config callbacks outside the lock (#741)

* fix(plugins): drop a plugin's package modules when it unloads

A plugin that keeps helpers in a package (providers/feed.py, imported as
`from providers.feed import ...`) leaves dotted entries in sys.modules.
PluginLoader only tracked bare names: `providers` was namespaced and
dropped on unload, `providers.feed` stayed. A reload after a store update
imported a fresh `providers`, then got the old `feed` back from the module
cache, so the new manager.py ran against the old helpers until the display
restarted. A load that failed part-way left them behind the same way.
Elections (providers/), flights (enrichment/) and olympics (data/,
renderers/) ship packages.

The loader now records the dotted modules whose file (or, for a namespace
package, every __path__ entry) lies inside the plugin directory. They keep
their names while the plugin runs, as before, and unregister_plugin_modules()
drops them, only while sys.modules still holds that plugin's module. The
failed-load cleanup in load_module() drops them too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(plugins): remove a symlinked dev plugin as a link

PluginStoreManager._safe_remove_directory, behind uninstall and behind
discarding the set-aside copy after an install or update, handed a
symlinked dev plugin (scripts/dev/dev_plugin_setup.sh) to shutil.rmtree,
which refuses a symlink. The chmod fallback then walked through the link
and set every directory and file in the linked checkout to 0700, and the
sudo stage refused the resolved path as outside the plugins directory. The
removal failed, the link stayed, and the developer's checkout lost its
group/other permissions. A dangling link read as already removed, because
exists() follows it, and was left behind.

A symlink is now unlinked before any other stage runs, and before the
exists() check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(plugins): load a dev plugin linked in under a different name

contained_plugin_dir(), the containment check before a plugin's
dependencies are installed, resolved the plugin directory and looked for
the resolved folder's name among the plugins directory's entries. A dev
plugin symlinked in under its id by a name its checkout does not share --
`dev_plugin_setup.sh link-github foo <url>` clones ledmatrix-foo, the
repository naming convention, and links it as plugins/foo -- has no such
entry, so install_dependencies() returned False and the load failed with
"Dependency installation failed", even with no requirements.txt.

When the path sits directly in the plugins directory, the entry it names
(the link) is looked up first; anything else is resolved and matched by
name as before. The answer is still always rebuilt from a name os.scandir()
returned for the plugins directory, so a path outside it is still refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(plugins): release a plugin whose update() raises a BaseException

On the async update worker, the wrapped update() finished its bookkeeping
(_finish: release the plugin lock, drop the pending slot, state back to
ENABLED) only for an Exception. asyncio.CancelledError and SystemExit
derive from BaseException, so one raised from update() skipped _finish:
the plugin kept its lock and stayed RUNNING for the life of the process,
never rescheduled, with every display() skipped as busy. PluginExecutor
caught only Exception as well, so its thread died with the call never
marked complete and an immediate failure was logged and recorded as a
timeout.

_target_update now runs _finish for any BaseException and re-raises it,
and the executor's thread stores it like any other exception, so it is
reported as the operation's failure (PluginError) on both the async and
the synchronous path. _finish and _record_update_failure take a
BaseException.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(config): notify config subscribers outside the service lock

ConfigService._load_config ran every subscriber while holding _lock. The
display's per-plugin subscriber calls PluginManager.apply_config_change,
which waits up to PLUGIN_LOCK_TIMEOUT (5 s) for a plugin busy in update().
A save that enables or disables a plugin also flags a reconcile, which the
render thread runs: its get_config(), and the unsubscribe() of a plugin it
disables, both take _lock, so the panel froze behind every slow callback,
up to 5 s per busy plugin.

The config is now swapped under _lock and the subscribers are called after
it is released, from a copy of the subscriber lists. A separate
_notify_lock is held across a whole reload (read, swap, notify), so one
reload's notifications still finish before the next one's start. Each
callback is checked against the live lists just before it runs, and
unsubscribe() waits only for a call of that same callback already in
progress (unless it is that callback's own thread), so a callback it
removed is not running and will not run once it returns, as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

src/config_service.py

@@ -14,7 +14,7 @@ import json
 import time
 import threading
 from pathlib import Path
-from typing import Dict, Any, Optional, List, Callable
+from typing import Dict, Any, Optional, List, Callable, Tuple
 from collections import defaultdict
 import logging
 import hashlib
@@ -52,7 +52,18 @@ class ConfigService:
         
         # Thread safety
         self._lock: threading.RLock = threading.RLock()
-        
+        # Held across a whole reload -- read, swap, notify -- so one reload's
+        # notifications finish before the next one's start. Subscribers run
+        # under this lock and never under _lock: the display's per-plugin
+        # subscriber can wait seconds for a busy plugin, and get_config(),
+        # subscribe() and unsubscribe() -- called from the render thread --
+        # must not wait behind it.
+        self._notify_lock: threading.RLock = threading.RLock()
+        # (key, callback, thread id) of the callback a notification is running,
+        # so unsubscribe() can wait for that one call; signalled on its return.
+        self._running_callback: Optional[Tuple[str, Callable[..., None], int]] = None
+        self._callback_done = threading.Condition(self._lock)
+
         # Current configuration
         self._current_config: Dict[str, Any] = {}
         self._current_checksum: Optional[str] = None
@@ -87,32 +98,33 @@ class ConfigService:
             True if config changed, False otherwise
         """
         try:
-            new_config = self.config_manager.load_config()
-            new_checksum = self._calculate_checksum(new_config)
-            
-            with self._lock:
-                # Check if config actually changed
-                if new_checksum == self._current_checksum:
-                    self.logger.debug("Configuration unchanged, skipping reload")
-                    return False
-                
-                # Store old config for change detection
-                old_config = self._current_config.copy()
-                
-                # Update current config
-                self._current_config = new_config
-                self._current_checksum = new_checksum
-                
-                # Notify subscribers
+            with self._notify_lock:
+                new_config = self.config_manager.load_config()
+                new_checksum = self._calculate_checksum(new_config)
+
+                with self._lock:
+                    # Check if config actually changed
+                    if new_checksum == self._current_checksum:
+                        self.logger.debug("Configuration unchanged, skipping reload")
+                        return False
+
+                    # Store old config for change detection
+                    old_config = self._current_config.copy()
+
+                    # Update current config
+                    self._current_config = new_config
+                    self._current_checksum = new_checksum
+
+                # Notify subscribers, outside _lock (see _notify_lock)
                 self._notify_subscribers(old_config, new_config)
-                
+
                 self.logger.info(
                     "Configuration reloaded (checksum: %s)",
                     new_checksum[:8]
                 )
-                
+
                 return True
-                
+
         except ConfigError as e:
             self.logger.error("Error loading configuration: %s", e, exc_info=True)
             return False
@@ -127,35 +139,64 @@ class ConfigService:
         Args:
             old_config: Previous configuration
             new_config: New configuration
+
+        Called without _lock held. The subscriber lists are copied under it,
+        and each callback is checked against them again just before it runs.
         """
+        with self._lock:
+            subscribers = {key: list(callbacks) for key, callbacks in self._subscribers.items()}
+
         # Notify global subscribers (key: '*')
-        for callback in self._subscribers.get('*', []):
-            try:
-                callback(old_config, new_config)
-            except Exception as e:
-                self.logger.error("Error in global config change callback: %s", e, exc_info=True)
-        
+        for callback in subscribers.get('*', []):
+            self._call_subscriber('*', callback, old_config, new_config)
+
         # Notify plugin-specific subscribers
-        for plugin_id in self._subscribers.keys():
+        for plugin_id, callbacks in subscribers.items():
             if plugin_id == '*':
                 continue
-            
+
             old_plugin_config = old_config.get(plugin_id, {})
             new_plugin_config = new_config.get(plugin_id, {})
-            
+
             # Only notify if plugin config actually changed
             if old_plugin_config != new_plugin_config:
-                for callback in self._subscribers[plugin_id]:
-                    try:
-                        callback(old_plugin_config, new_plugin_config)
-                    except Exception as e:
-                        self.logger.error(
-                            "Error in config change callback for %s: %s",
-                            plugin_id,
-                            e,
-                            exc_info=True
-                        )
-    
+                for callback in callbacks:
+                    self._call_subscriber(plugin_id, callback,
+                                          old_plugin_config, new_plugin_config)
+
+    def _call_subscriber(
+        self,
+        key: str,
+        callback: Callable[[Dict[str, Any], Dict[str, Any]], None],
+        old_config: Dict[str, Any],
+        new_config: Dict[str, Any],
+    ) -> None:
+        """Run one callback, unless it was unsubscribed since the snapshot.
+
+        unsubscribe() promises that once it returns the callback is neither
+        running nor will run: the display unloads the plugin straight after.
+        """
+        with self._lock:
+            if callback not in self._subscribers.get(key, ()):
+                return
+            self._running_callback = (key, callback, threading.get_ident())
+        try:
+            callback(old_config, new_config)
+        except Exception as e:
+            if key == '*':
+                self.logger.error("Error in global config change callback: %s", e, exc_info=True)
+            else:
+                self.logger.error(
+                    "Error in config change callback for %s: %s",
+                    key,
+                    e,
+                    exc_info=True
+                )
+        finally:
+            with self._lock:
+                self._running_callback = None
+                self._callback_done.notify_all()
+
     def _check_file_changes(self) -> bool:
         """
         Check if configuration files have been modified.
@@ -276,6 +317,11 @@ class ConfigService:
         """
         Unsubscribe from configuration changes.
         
+        Once this returns the callback is not running and will not be called
+        again. A notification that is running this very callback is waited
+        for (unless the callback is the caller); one running any other
+        callback is not.
+
         Args:
             callback: Callback function to remove
             plugin_id: Optional plugin ID (must match subscription)
@@ -285,6 +331,10 @@ class ConfigService:
             if callback in self._subscribers[key]:
                 self._subscribers[key].remove(callback)
                 self.logger.debug("Unsubscribed from config changes for %s", key)
+            while (self._running_callback is not None
+                   and self._running_callback[:2] == (key, callback)
+                   and self._running_callback[2] != threading.get_ident()):
+                self._callback_done.wait()
     
     def shutdown(self) -> None:
         """Shutdown the configuration service."""

src/plugin_system/plugin_executor.py

@@ -92,7 +92,10 @@ class PluginExecutor:
                 with plugin_scope(plugin_id):
                     result_container['value'] = operation()
                 result_container['completed'] = True
-            except Exception as e:
+            except BaseException as e:  # pylint: disable=broad-except
+                # asyncio.CancelledError and SystemExit too: uncaught, one
+                # ended this thread with 'completed' unset, and an operation
+                # that failed at once was reported as timing out.
                 result_container['exception'] = e
                 result_container['completed'] = True
         

src/plugin_system/plugin_loader.py

@@ -199,9 +199,22 @@ def contained_plugin_dir(plugin_dir: Path, plugins_dir: Path) -> Optional[str]:
     name that came out of ``os.scandir()`` on the trusted root carries no
     taint, which is a real containment guarantee (and one CodeQL's
     path-injection query can follow), not a string sanitiser.
+
+    The entry looked for is the one ``plugin_dir`` itself names when it sits
+    directly in ``plugins_dir``: for a dev plugin symlinked in under its id,
+    the link's name. Resolving the link first and looking for the target's
+    folder name refused ``plugins/foo -> ~/.ledmatrix-dev-plugins/ledmatrix-foo``
+    (what ``dev_plugin_setup.sh link-github foo <url>`` makes), so the plugin
+    never loaded. Any other path is resolved and matched by its final name,
+    as before.
     """
-    plugin_dir_real = os.path.realpath(str(plugin_dir))
     plugins_dir_real = os.path.realpath(str(plugins_dir))
+    plugin_dir_abs = os.path.abspath(str(plugin_dir))
+    if os.path.realpath(os.path.dirname(plugin_dir_abs)) == plugins_dir_real:
+        matched_name = find_trusted_subdir(plugins_dir_real, os.path.basename(plugin_dir_abs))
+        if matched_name is not None:
+            return os.path.join(plugins_dir_real, matched_name)
+    plugin_dir_real = os.path.realpath(str(plugin_dir))
     matched_name = find_trusted_subdir(plugins_dir_real, os.path.basename(plugin_dir_real))
     if matched_name is None:
         return None
@@ -243,6 +256,10 @@ class PluginLoader:
         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
+        # plugin_id -> {dotted name: module} for the modules of the plugin's
+        # own packages (``providers.feed``). They keep their names while the
+        # plugin runs and are dropped with it; see _iter_plugin_submodules.
+        self._plugin_submodules: Dict[str, Dict[str, Any]] = {}
         # Lock to serialize module loading when plugins share module names
         # (e.g., scroll_display.py, game_renderer.py across sport plugins).
         # During exec_module, bare-name sub-modules temporarily appear in
@@ -449,6 +466,45 @@ class PluginLoader:
                 continue
         return result
 
+    @staticmethod
+    def _iter_plugin_submodules(
+        plugin_dir: Path, before_keys: set
+    ) -> list:
+        """Return dotted-name modules from plugin_dir added after before_keys.
+
+        The modules of a package the plugin ships (``providers.feed`` from
+        ``providers/feed.py``). _iter_plugin_bare_modules skips them, so the
+        bare ``providers`` was namespaced and dropped on unload while
+        ``providers.feed`` stayed in sys.modules: a reload after a store update
+        imported a fresh ``providers`` and then got the old ``feed`` back from
+        the cache, running the new manager.py against the old helpers until the
+        display restarted.
+
+        A module counts when its ``__file__`` -- or, for a namespace package,
+        which has none, every ``__path__`` entry -- is inside plugin_dir, so a
+        library the plugin imports (``requests.adapters``) never does.
+
+        Returns a list of (mod_name, module) tuples.
+        """
+        resolved_dir = plugin_dir.resolve()
+        result = []
+        for key in set(sys.modules.keys()) - before_keys:
+            if "." not in key:
+                continue
+            mod = sys.modules.get(key)
+            if mod is None:
+                continue
+            mod_file = getattr(mod, "__file__", None)
+            locations = [mod_file] if mod_file else list(getattr(mod, "__path__", None) or [])
+            if not locations:
+                continue
+            try:
+                if all(Path(loc).resolve().is_relative_to(resolved_dir) for loc in locations):
+                    result.append((key, mod))
+            except (ValueError, TypeError, OSError):
+                continue
+        return result
+
     def _evict_stale_bare_modules(self, plugin_dir: Path) -> dict:
         """Temporarily remove bare-name sys.modules entries from other plugins.
 
@@ -527,6 +583,13 @@ class PluginLoader:
         # Track for cleanup during unload
         self._plugin_module_registry[plugin_id] = namespaced_names
 
+        # The modules of the plugin's own packages keep their dotted names
+        # while it runs -- as they always have, so the package and its
+        # children stay a matching set in sys.modules -- and are dropped
+        # with the plugin by unregister_plugin_modules().
+        self._plugin_submodules[plugin_id] = dict(
+            self._iter_plugin_submodules(plugin_dir, before_keys))
+
         if namespaced_names:
             self.logger.info(
                 "Namespace-isolated %d module(s) for plugin %s",
@@ -537,10 +600,16 @@ class PluginLoader:
         """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.
+        that were created when the plugin was loaded, including the dotted
+        modules of its packages. A dotted name is dropped only while it still
+        holds this plugin's module: the name is not namespaced, so another
+        plugin may have put its own there since.
         """
         for ns_name in self._plugin_module_registry.pop(plugin_id, set()):
             sys.modules.pop(ns_name, None)
+        for name, mod in self._plugin_submodules.pop(plugin_id, {}).items():
+            if sys.modules.get(name) is mod:
+                sys.modules.pop(name, None)
         self._loaded_modules.pop(plugin_id, None)
 
     def load_module(
@@ -646,11 +715,13 @@ class PluginLoader:
                     if evicted_name not in sys.modules:
                         sys.modules[evicted_name] = evicted_mod
                 # 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.
+                # bare-name or package 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 self._iter_plugin_bare_modules(plugin_dir, before_keys):
                     sys.modules.pop(key, None)
+                for key, _ in self._iter_plugin_submodules(plugin_dir, before_keys):
+                    sys.modules.pop(key, None)
                 raise
 
             self._loaded_modules[plugin_id] = module

src/plugin_system/plugin_manager.py

@@ -1163,7 +1163,7 @@ class PluginManager:
     def _record_update_failure(
         self,
         plugin_id: str,
-        exc: Optional[Exception] = None,
+        exc: Optional[BaseException] = None,
         log: bool = True,
         count_failure: bool = True,
     ) -> None:
@@ -1187,7 +1187,7 @@ class PluginManager:
         """
         failure_time = time.time()
         if exc is not None:
-            err: Exception = exc
+            err: BaseException = exc
             error_type = type(exc).__name__
         else:
             err = Exception(f"Plugin {plugin_id} execution failed (timeout or executor error)")
@@ -1653,7 +1653,7 @@ class PluginManager:
         finish_guard = threading.Lock()
         finished = {'done': False}
 
-        def _finish(success: bool, exc: Optional[Exception] = None) -> None:
+        def _finish(success: bool, exc: Optional[BaseException] = None) -> None:
             with finish_guard:
                 if finished['done']:
                     return
@@ -1727,7 +1727,13 @@ class PluginManager:
                     self.resource_monitor.monitor_call(plugin_id, plugin_instance.update)
                 else:
                     plugin_instance.update()
-            except Exception as exc:
+            except BaseException as exc:  # pylint: disable=broad-except
+                # BaseException, not just Exception: asyncio.CancelledError
+                # and SystemExit derive from it. Either one skipped _finish,
+                # so the plugin kept its lock and stayed RUNNING for good --
+                # never rescheduled, and every display() skipped as busy.
+                # Re-raised for the executor, which reports it as this
+                # update's failure.
                 _finish(False, exc=exc)
                 raise
             else:

src/plugin_system/store_manager.py

@@ -344,12 +344,26 @@ class PluginStoreManager(_RegistryMixin, _InstallMixin, _UpdateMixin):
         2. Fix permissions via os.chmod() then retry (works for same-owner files)
         3. Use sudo rm -rf as last resort (works for root-owned __pycache__, etc.)
 
+        A symlink -- a dev plugin linked in by scripts/dev/dev_plugin_setup.sh
+        -- is removed as a link, before any of that: rmtree refuses one, and
+        stage 2 would walk through it and chmod the developer's checkout.
+
         Args:
             path: Path to directory to remove
 
         Returns:
             True if directory was removed successfully, False otherwise
         """
+        if path.is_symlink():
+            # Checked before exists(), which follows the link: a dangling one
+            # would read as already removed and be left behind.
+            try:
+                path.unlink()
+                return True
+            except OSError as e:
+                self.logger.error(f"Could not remove the symlink {path}: {e}")
+                return False
+
         if not path.exists():
             return True  # Already removed