From 6428bd3e87211a61280efa8da96ab93493af15a1 Mon Sep 17 00:00:00 2001 From: Ron Date: Tue, 16 Jun 2026 18:03:24 -0700 Subject: [PATCH 1/3] feat(display-controller): hot-reload plugin enable/disable without a restart Enabling or disabling a plugin in config previously required restarting the display service: the plugin list and available_modes were built once at init and the run loop never revisited them. (Per-plugin config *values* already hot-reloaded; only the enabled set was restart-only.) Now the controller reconciles its running plugins against the config's enabled set whenever that set changes: - The ConfigService watcher thread only sets a `_pending_plugin_reconcile` flag (via a cheap enabled-set diff). It never mutates loop state. - The run loop applies the reconcile on its own thread (top of each iteration, deferred while on-demand is active), so loading/unloading and rebuilding available_modes can't race with rendering. - `_reconcile_enabled_plugins` diffs desired vs running plugins, unloads the removed ones (cleanup + on_disable + config-unsubscribe via the new `_unregister_plugin`) and loads the added ones, then clamps the rotation index so the current mode stays valid. The per-plugin registration done at startup is extracted into `_register_loaded_plugin` and reused by the live-enable path so both build identical state. Extracting it also fixes a latent late-binding bug: the per-plugin config-change callbacks were closures over the loop variable, so every plugin's callback targeted the last-loaded instance; each now binds its own id/instance. Adds test/test_display_controller_plugin_toggle.py covering live enable, live disable, index clamping, no-op when unchanged, and the enabled-set diff. Co-Authored-By: Claude Opus 4.8 --- src/display_controller.py | 214 ++++++++++++++---- test/test_display_controller_plugin_toggle.py | 157 +++++++++++++ 2 files changed, 329 insertions(+), 42 deletions(-) create mode 100644 test/test_display_controller_plugin_toggle.py diff --git a/src/display_controller.py b/src/display_controller.py index 4ebe580a..da70a5e9 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -24,7 +24,7 @@ import os import json from pathlib import Path -from typing import Dict, Any, List, Optional +from typing import Dict, Any, List, Optional, Callable from datetime import datetime from concurrent.futures import ThreadPoolExecutor, as_completed # pylint: disable=no-name-in-module import pytz @@ -163,6 +163,13 @@ def _follower_gated_update(): self.plugin_modes = {} # mode -> plugin_instance mapping for plugin-first dispatch self.mode_to_plugin_id: Dict[str, str] = {} self.plugin_display_modes: Dict[str, List[str]] = {} + # Per-plugin config-change callbacks, kept so we can unsubscribe a + # plugin when it is disabled live. + self._plugin_config_callbacks: Dict[str, Callable] = {} + # Set by the config-watcher thread when the enabled-plugin set changes; + # the main run loop reconciles (loads/unloads) on its own thread so + # mutating available_modes never races with rendering. + self._pending_plugin_reconcile = False self.on_demand_active = False self.on_demand_mode: Optional[str] = None self.on_demand_modes: List[str] = [] # All modes for the on-demand plugin @@ -331,47 +338,10 @@ def load_single_plugin(plugin_id): logger.info("✓ Loaded plugin %s in %.3f seconds (%d/%d)", plugin_id, result['load_time'], loaded_count, enabled_count) - # Get plugin instance and manifest - plugin_instance = self.plugin_manager.get_plugin(plugin_id) - manifest = self.plugin_manager.plugin_manifests.get(plugin_id, {}) - - # Prefer plugin's modes attribute if available (dynamic based on enabled leagues) - # Fall back to manifest display_modes if plugin doesn't provide modes - if plugin_instance and hasattr(plugin_instance, 'modes') and plugin_instance.modes: - display_modes = list(plugin_instance.modes) - logger.debug("Using plugin.modes for %s: %s", plugin_id, display_modes) - else: - display_modes = manifest.get('display_modes', [plugin_id]) - logger.debug("Using manifest display_modes for %s: %s", plugin_id, display_modes) - - if isinstance(display_modes, list) and display_modes: - self.plugin_display_modes[plugin_id] = list(display_modes) - else: - display_modes = [plugin_id] - self.plugin_display_modes[plugin_id] = list(display_modes) - - # Subscribe plugin to config changes for hot-reload - if hasattr(self, 'config_service') and hasattr(plugin_instance, 'on_config_change'): - def config_change_callback(old_config: Dict[str, Any], new_config: Dict[str, Any]) -> None: - """Callback for plugin config changes.""" - try: - plugin_instance.on_config_change(new_config) - logger.debug("Plugin %s notified of config change", plugin_id) - except Exception as e: - logger.error("Error in plugin %s config change handler: %s", plugin_id, e, exc_info=True) - - self.config_service.subscribe(config_change_callback, plugin_id=plugin_id) - logger.debug("Subscribed plugin %s to config changes", plugin_id) - - # Add plugin modes to available modes - for mode in display_modes: - self.available_modes.append(mode) - self.plugin_modes[mode] = plugin_instance - self.mode_to_plugin_id[mode] = plugin_id - logger.debug(" Added mode: %s", mode) - # Invalidate signature cache so the new instance is re-inspected - self._plugin_accepts_display_mode.pop(plugin_id, None) - + # Register the loaded plugin's modes, config subscription + # and dispatch maps (shared with live enable hot-reload). + self._register_loaded_plugin(plugin_id) + # Show progress progress_pct = int((loaded_count / enabled_count) * 100) elapsed = time.time() - plugin_time @@ -447,6 +417,10 @@ def config_change_callback(old_config: Dict[str, Any], new_config: Dict[str, Any # when the user saves settings via the web UI. def _controller_config_change(old_config: Dict[str, Any], new_config: Dict[str, Any]) -> None: self._refresh_config_cache(new_config) + # If a plugin was enabled/disabled, flag a reconcile for the main + # loop to apply (loading/unloading off the watcher thread is unsafe). + if self._enabled_set_changed(old_config, new_config): + self._pending_plugin_reconcile = True self.config_service.subscribe(_controller_config_change) @@ -1582,6 +1556,15 @@ def run(self): logger.info(f"Initial mode set to: {self.current_display_mode} (index: {self.current_mode_index}, total modes: {len(self.available_modes)})") while True: + # Apply plugin enable/disable edits saved via the web UI. The + # config-watcher thread only sets the flag; loading/unloading and + # rebuilding available_modes happens here on the render thread so + # it can't race with rendering. Deferred while on-demand is active + # (the flag stays set) so we don't fight its temporary-enable. + if self._pending_plugin_reconcile and not self.on_demand_active: + self._pending_plugin_reconcile = False + self._reconcile_enabled_plugins() + # Handle on-demand commands before rendering self._poll_on_demand_requests() self._check_on_demand_expiration() @@ -2507,6 +2490,153 @@ def _cleanup_expired_wifi_status(self): self.wifi_status_active = False self.wifi_status_expires_at = None + def _register_loaded_plugin(self, plugin_id: str) -> List[str]: + """Register an already-loaded plugin's display modes, config-change + subscription and dispatch maps with the controller. + + Shared by startup loading and live enable hot-reload so both paths + build identical controller state. Returns the registered modes. + """ + plugin_instance = self.plugin_manager.get_plugin(plugin_id) + manifest = self.plugin_manager.plugin_manifests.get(plugin_id, {}) + + # Prefer the plugin's dynamic modes attribute (e.g. based on enabled + # leagues), else fall back to manifest display_modes, else the id. + if plugin_instance is not None and getattr(plugin_instance, 'modes', None): + display_modes = list(plugin_instance.modes) + logger.debug("Using plugin.modes for %s: %s", plugin_id, display_modes) + else: + display_modes = manifest.get('display_modes', [plugin_id]) + logger.debug("Using manifest display_modes for %s: %s", plugin_id, display_modes) + if not (isinstance(display_modes, list) and display_modes): + display_modes = [plugin_id] + self.plugin_display_modes[plugin_id] = list(display_modes) + + # Subscribe to config changes for per-plugin hot-reload. Bind plugin_id + # and instance as defaults so each plugin's callback targets its own + # instance (avoids late-binding when registering many plugins), and + # remember the callback so we can unsubscribe on disable. + if hasattr(self, 'config_service') and hasattr(plugin_instance, 'on_config_change'): + def config_change_callback(old_config: Dict[str, Any], new_config: Dict[str, Any], + _pid: str = plugin_id, _plugin: Any = plugin_instance) -> None: + """Callback for plugin config changes.""" + try: + _plugin.on_config_change(new_config) + logger.debug("Plugin %s notified of config change", _pid) + except Exception as e: + logger.error("Error in plugin %s config change handler: %s", _pid, e, exc_info=True) + + self.config_service.subscribe(config_change_callback, plugin_id=plugin_id) + self._plugin_config_callbacks[plugin_id] = config_change_callback + logger.debug("Subscribed plugin %s to config changes", plugin_id) + + # Add modes to the dispatch maps. + for mode in display_modes: + if mode not in self.available_modes: + self.available_modes.append(mode) + self.plugin_modes[mode] = plugin_instance + self.mode_to_plugin_id[mode] = plugin_id + logger.debug(" Added mode: %s", mode) + # Invalidate signature cache so the new instance is re-inspected. + self._plugin_accepts_display_mode.pop(plugin_id, None) + return display_modes + + def _unregister_plugin(self, plugin_id: str) -> None: + """Remove a plugin's modes, config subscription and instance, then + unload it. Used by live disable hot-reload.""" + modes = self.plugin_display_modes.pop(plugin_id, []) + for mode in modes: + if mode in self.available_modes: + self.available_modes.remove(mode) + self.plugin_modes.pop(mode, None) + self.mode_to_plugin_id.pop(mode, None) + + # Unsubscribe the plugin's config-change callback. + callback = self._plugin_config_callbacks.pop(plugin_id, None) + if callback is not None and hasattr(self, 'config_service'): + try: + self.config_service.unsubscribe(callback, plugin_id=plugin_id) + except Exception as e: + logger.debug("Error unsubscribing plugin %s from config changes: %s", plugin_id, e) + + self._plugin_accepts_display_mode.pop(plugin_id, None) + + # Tear down the instance (cleanup + on_disable + module unload). + try: + self.plugin_manager.unload_plugin(plugin_id) + except Exception as e: + logger.error("Error unloading plugin %s: %s", plugin_id, e, exc_info=True) + + logger.info("Disabled plugin %s live (removed modes: %s)", plugin_id, modes) + + def _enabled_set_changed(self, old_config: Dict[str, Any], new_config: Dict[str, Any]) -> bool: + """True if any top-level section's ``enabled`` flag differs between two + configs. A cheap watcher-thread check that gates the full reconcile. + Non-plugin sections (e.g. schedule) may match too; the reconcile + no-ops for anything that isn't a discovered plugin.""" + def enabled_map(cfg: Dict[str, Any]) -> Dict[str, bool]: + return { + key: bool(value.get('enabled', False)) + for key, value in cfg.items() + if isinstance(value, dict) + } + return enabled_map(old_config) != enabled_map(new_config) + + def _reconcile_enabled_plugins(self) -> None: + """Load/unload plugins so the running set matches the enabled set in + config. Runs on the main display thread (never the config-watcher + thread) so mutating available_modes is race-free against rendering.""" + if self.plugin_manager is None: + return + try: + config = self.config_service.get_config() + except Exception: + config = self.config + try: + discovered = set(self.plugin_manager.discover_plugins()) + except Exception as e: + logger.error("Plugin reconcile: discovery failed: %s", e, exc_info=True) + return + + desired = {p for p in discovered if config.get(p, {}).get('enabled', False)} + current = set(self.plugin_display_modes.keys()) + to_add = desired - current + to_remove = current - desired + if not to_add and not to_remove: + return + + previous_mode = self.current_display_mode + + for plugin_id in to_remove: + self._unregister_plugin(plugin_id) + + for plugin_id in to_add: + try: + if self.plugin_manager.load_plugin(plugin_id): + modes = self._register_loaded_plugin(plugin_id) + logger.info("Enabled plugin %s live (modes: %s)", plugin_id, modes) + else: + logger.warning("Plugin reconcile: failed to load %s", plugin_id) + except Exception as e: + logger.error("Plugin reconcile: error enabling %s: %s", plugin_id, e, exc_info=True) + + self._resync_mode_index_after_change(previous_mode) + logger.info("Plugin reconcile complete: +%s -%s (%d modes)", + sorted(to_add), sorted(to_remove), len(self.available_modes)) + + def _resync_mode_index_after_change(self, previous_mode: Optional[str]) -> None: + """Clamp rotation state after available_modes changed. Stays on the + previous mode if it survived, otherwise restarts cleanly within range.""" + if not self.available_modes: + self.current_mode_index = 0 + self.current_display_mode = None + return + if previous_mode in self.available_modes: + self.current_mode_index = self.available_modes.index(previous_mode) + else: + self.current_mode_index %= len(self.available_modes) + self.current_display_mode = self.available_modes[self.current_mode_index] + def _refresh_config_cache(self, new_config: Dict[str, Any]) -> None: """Refresh all config-derived caches when a hot-reload fires. diff --git a/test/test_display_controller_plugin_toggle.py b/test/test_display_controller_plugin_toggle.py new file mode 100644 index 00000000..b729a03d --- /dev/null +++ b/test/test_display_controller_plugin_toggle.py @@ -0,0 +1,157 @@ +"""Tests for live plugin enable/disable hot-reload in DisplayController. + +Enabling or disabling a plugin in config used to require a full display +restart because the plugin list and available_modes were built once at init. +These tests cover the reconcile path that loads/unloads plugins and rebuilds +the dispatch maps on the main thread when the enabled set changes. +""" + +from unittest.mock import MagicMock + + +def _make_plugin(modes): + plugin = MagicMock() + plugin.modes = list(modes) + return plugin + + +def _wire_plugin_manager(controller, plugins, discovered=None): + """Point the controller's mock plugin_manager at a set of fake plugins. + + `plugins` maps plugin_id -> mock instance (with a .modes list). + """ + pm = controller.plugin_manager + pm.discover_plugins.return_value = list(discovered if discovered is not None else plugins.keys()) + pm.load_plugin.return_value = True + pm.unload_plugin.return_value = True + pm.plugin_manifests = {} + pm.get_plugin.side_effect = lambda pid: plugins.get(pid) + return pm + + +def _set_config(controller, cfg): + controller.config_service.get_config = lambda: cfg + + +class TestPluginEnableDisableHotReload: + def test_enable_plugin_live(self, test_display_controller): + controller = test_display_controller + assert controller.available_modes == [] + + plugin = _make_plugin(["foo"]) + _wire_plugin_manager(controller, {"foo": plugin}) + _set_config(controller, {"foo": {"enabled": True}}) + + controller._reconcile_enabled_plugins() + + assert "foo" in controller.plugin_display_modes + assert "foo" in controller.available_modes + assert controller.plugin_modes["foo"] is plugin + assert controller.mode_to_plugin_id["foo"] == "foo" + controller.plugin_manager.load_plugin.assert_any_call("foo") + + def test_disable_plugin_live(self, test_display_controller): + controller = test_display_controller + plugin = _make_plugin(["live", "recent"]) + _wire_plugin_manager(controller, {"sports": plugin}, discovered=["sports"]) + + # Enable, then disable. + _set_config(controller, {"sports": {"enabled": True}}) + controller._reconcile_enabled_plugins() + assert "sports" in controller.plugin_display_modes + assert "live" in controller.available_modes and "recent" in controller.available_modes + assert "sports" in controller._plugin_config_callbacks + + _set_config(controller, {"sports": {"enabled": False}}) + controller._reconcile_enabled_plugins() + + assert "sports" not in controller.plugin_display_modes + assert "live" not in controller.available_modes + assert "recent" not in controller.available_modes + assert "live" not in controller.plugin_modes + assert "recent" not in controller.mode_to_plugin_id + controller.plugin_manager.unload_plugin.assert_any_call("sports") + assert "sports" not in controller._plugin_config_callbacks + + def test_disable_clamps_current_mode_index(self, test_display_controller): + controller = test_display_controller + p1 = _make_plugin(["a"]) + p2 = _make_plugin(["b"]) + _wire_plugin_manager(controller, {"p1": p1, "p2": p2}, discovered=["p1", "p2"]) + + _set_config(controller, {"p1": {"enabled": True}, "p2": {"enabled": True}}) + controller._reconcile_enabled_plugins() + # Add order across multiple plugins is set-driven (as at init), so + # compare membership, not order. + assert set(controller.available_modes) == {"a", "b"} + + # Pretend we're currently showing p2's mode. + controller.current_mode_index = controller.available_modes.index("b") + controller.current_display_mode = "b" + + _set_config(controller, {"p1": {"enabled": True}, "p2": {"enabled": False}}) + controller._reconcile_enabled_plugins() + + assert controller.available_modes == ["a"] + # Index must be back in range and the display mode no longer the removed one. + assert 0 <= controller.current_mode_index < len(controller.available_modes) + assert controller.current_display_mode == "a" + + def test_enable_keeps_current_mode(self, test_display_controller): + controller = test_display_controller + p1 = _make_plugin(["a"]) + p2 = _make_plugin(["b"]) + _wire_plugin_manager(controller, {"p1": p1, "p2": p2}, discovered=["p1", "p2"]) + + _set_config(controller, {"p1": {"enabled": True}}) + controller._reconcile_enabled_plugins() + controller.current_mode_index = 0 + controller.current_display_mode = "a" + + # Enabling p2 should not disturb the currently-showing mode. + _set_config(controller, {"p1": {"enabled": True}, "p2": {"enabled": True}}) + controller._reconcile_enabled_plugins() + + assert "b" in controller.available_modes + assert controller.current_display_mode == "a" + assert controller.available_modes[controller.current_mode_index] == "a" + + def test_noop_when_enabled_set_unchanged(self, test_display_controller): + controller = test_display_controller + plugin = _make_plugin(["foo"]) + _wire_plugin_manager(controller, {"foo": plugin}, discovered=["foo"]) + _set_config(controller, {"foo": {"enabled": True}}) + controller._reconcile_enabled_plugins() + + load_calls = controller.plugin_manager.load_plugin.call_count + unload_calls = controller.plugin_manager.unload_plugin.call_count + + # Reconcile again with no change — must not load/unload anything. + controller._reconcile_enabled_plugins() + assert controller.plugin_manager.load_plugin.call_count == load_calls + assert controller.plugin_manager.unload_plugin.call_count == unload_calls + + +class TestEnabledSetChanged: + def test_detects_toggle(self, test_display_controller): + c = test_display_controller + assert c._enabled_set_changed({"a": {"enabled": True}}, {"a": {"enabled": False}}) is True + + def test_no_change(self, test_display_controller): + c = test_display_controller + cfg = {"a": {"enabled": True}, "b": {"enabled": False}} + assert c._enabled_set_changed(cfg, dict(cfg)) is False + + def test_new_enabled_section(self, test_display_controller): + c = test_display_controller + assert c._enabled_set_changed( + {"a": {"enabled": True}}, + {"a": {"enabled": True}, "b": {"enabled": True}}, + ) is True + + def test_ignores_non_enabled_value_edits(self, test_display_controller): + c = test_display_controller + assert c._enabled_set_changed( + {"a": {"enabled": True, "duration": 30}}, + {"a": {"enabled": True, "duration": 45}}, + ) is False From 66e3028a510a0b400c911e3a488cca8e0f63c02f Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Mon, 6 Jul 2026 13:59:42 -0400 Subject: [PATCH 2/3] fix(display-controller): don't exit on empty available_modes, guard rotation modulo Hot-reload means available_modes can legitimately be empty at startup (no plugins enabled yet) and become non-empty later via the web UI, or vice versa mid-run. Fix four issues found reviewing this PR: - run() exited the process entirely when available_modes was empty at startup instead of idling, permanently defeating the point of live enable/disable for anyone who starts with zero plugins enabled. - The mode-rotation step divided by len(available_modes) unconditionally, raising ZeroDivisionError if the last enabled plugin is disabled between frames. - _reconcile_enabled_plugins() called .get('enabled', False) on a config section without checking it was a dict first, raising AttributeError on a malformed config value. - Minor: pop the config-change callback only after attempting to unsubscribe it, and log the exception in the config-read fallback instead of swallowing it silently. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --- src/display_controller.py | 34 +++++++++++---- test/test_display_controller_plugin_toggle.py | 42 +++++++++++++++++++ 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/display_controller.py b/src/display_controller.py index da70a5e9..52a798fc 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -1545,10 +1545,11 @@ def _check_live_priority(self, advance=False): def run(self): """Run the display controller, switching between displays.""" if not self.available_modes: - logger.warning("No display modes are enabled. Exiting.") - self.display_manager.cleanup() - return - + logger.warning( + "No display modes are enabled at startup; idling until a " + "plugin is enabled via the web UI." + ) + try: # Initialize with cached data for fast startup - let background updates refresh naturally logger.info("Starting display with cached data (fast startup mode)") @@ -1565,6 +1566,14 @@ def run(self): self._pending_plugin_reconcile = False self._reconcile_enabled_plugins() + if not self.available_modes: + # Nothing to render yet; keep servicing plugin update + # schedules and the reconcile flag so we pick up a mode + # as soon as one is enabled via the web UI, instead of + # having exited already at startup. + self._sleep_with_plugin_updates(30) + continue + # Handle on-demand commands before rendering self._poll_on_demand_requests() self._check_on_demand_expiration() @@ -2302,7 +2311,7 @@ def _should_exit_dynamic(elapsed_time: float) -> bool: except Exception as e: logger.warning("Error checking live priority for %s: %s", active_mode, e) - if should_rotate: + if should_rotate and self.available_modes: self.current_mode_index = (self.current_mode_index + 1) % len(self.available_modes) self.current_display_mode = self.available_modes[self.current_mode_index] self.last_mode_change = time.time() @@ -2551,13 +2560,16 @@ def _unregister_plugin(self, plugin_id: str) -> None: self.plugin_modes.pop(mode, None) self.mode_to_plugin_id.pop(mode, None) - # Unsubscribe the plugin's config-change callback. - callback = self._plugin_config_callbacks.pop(plugin_id, None) + # Unsubscribe the plugin's config-change callback. Pop only after the + # unsubscribe attempt so a failed unsubscribe doesn't lose our only + # reference to the callback before config_service has processed it. + callback = self._plugin_config_callbacks.get(plugin_id) if callback is not None and hasattr(self, 'config_service'): try: self.config_service.unsubscribe(callback, plugin_id=plugin_id) except Exception as e: logger.debug("Error unsubscribing plugin %s from config changes: %s", plugin_id, e) + self._plugin_config_callbacks.pop(plugin_id, None) self._plugin_accepts_display_mode.pop(plugin_id, None) @@ -2590,7 +2602,8 @@ def _reconcile_enabled_plugins(self) -> None: return try: config = self.config_service.get_config() - except Exception: + except Exception as e: + logger.warning("Plugin reconcile: falling back to cached config: %s", e) config = self.config try: discovered = set(self.plugin_manager.discover_plugins()) @@ -2598,7 +2611,10 @@ def _reconcile_enabled_plugins(self) -> None: logger.error("Plugin reconcile: discovery failed: %s", e, exc_info=True) return - desired = {p for p in discovered if config.get(p, {}).get('enabled', False)} + desired = { + p for p in discovered + if isinstance(config.get(p), dict) and config.get(p, {}).get('enabled', False) + } current = set(self.plugin_display_modes.keys()) to_add = desired - current to_remove = current - desired diff --git a/test/test_display_controller_plugin_toggle.py b/test/test_display_controller_plugin_toggle.py index b729a03d..856c0450 100644 --- a/test/test_display_controller_plugin_toggle.py +++ b/test/test_display_controller_plugin_toggle.py @@ -131,6 +131,48 @@ def test_noop_when_enabled_set_unchanged(self, test_display_controller): assert controller.plugin_manager.load_plugin.call_count == load_calls assert controller.plugin_manager.unload_plugin.call_count == unload_calls + def test_reconcile_ignores_non_dict_config_value(self, test_display_controller): + """A malformed config value (e.g. a stray string where a plugin's + section should be a dict) must be treated as disabled, not crash + the reconcile with AttributeError.""" + controller = test_display_controller + plugin = _make_plugin(["foo"]) + _wire_plugin_manager(controller, {"foo": plugin}, discovered=["foo"]) + _set_config(controller, {"foo": "not-a-dict"}) + + controller._reconcile_enabled_plugins() # must not raise + + assert "foo" not in controller.plugin_display_modes + assert "foo" not in controller.available_modes + + +class TestRunWithNoModesEnabled: + """Before hot-reload, an empty available_modes at startup was permanent + -- the display never came back without a restart. Now that a plugin can + be enabled live from the web UI, run() must idle rather than exit.""" + + def test_idles_instead_of_exiting(self, test_display_controller): + controller = test_display_controller + assert controller.available_modes == [] + + sleep_calls = [] + + def fake_sleep(duration, tick_interval=1.0): + sleep_calls.append(duration) + if len(sleep_calls) >= 3: + # Stand in for the process being torn down; run() catches + # this via its broad except + finally, same as any other + # unexpected error during the loop. + raise RuntimeError("stop-test-loop") + + controller._sleep_with_plugin_updates = fake_sleep + + controller.run() + + # Old behavior returned before ever reaching the loop body, so + # _sleep_with_plugin_updates would never have been called. + assert sleep_calls == [30, 30, 30] + class TestEnabledSetChanged: def test_detects_toggle(self, test_display_controller): From 77cef628164f71a2664076febe6ee6b4dd76ee4d Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Mon, 6 Jul 2026 15:57:54 -0400 Subject: [PATCH 3/3] fix(display-controller): address review findings on the hot-reload PR - Idle-wait tick was a fixed 30s sleep, delaying pickup of a plugin enabled via the web UI while no modes were active. Shortened to ~1s so it's roughly as responsive as the per-frame check once modes exist. - _unregister_plugin popped the config-change callback from _plugin_config_callbacks even when config_service.unsubscribe() raised, losing the only reference to it. Now only pops on a successful unsubscribe. - _pending_plugin_reconcile was cleared before _reconcile_enabled_plugins() ran, so a retryable failure (e.g. plugin discovery erroring) silently dropped the enable/disable request. _reconcile_enabled_plugins() now returns True/False and the caller only clears the flag on True. - Added a warning log for the malformed-config case (a plugin's config section present but not a dict) so it's actually visible, and updated the existing test to assert it via caplog. Left the broad `except Exception` around config_service.unsubscribe() as Exception -- the current implementation is a simple lock+dict/list op that doesn't document or realistically raise a narrower type, so this is a defensive catch-all, not user error handling. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --- src/display_controller.py | 50 +++++++++----- test/test_display_controller_plugin_toggle.py | 66 +++++++++++++++++-- 2 files changed, 95 insertions(+), 21 deletions(-) diff --git a/src/display_controller.py b/src/display_controller.py index 52a798fc..905257e4 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -1563,15 +1563,17 @@ def run(self): # it can't race with rendering. Deferred while on-demand is active # (the flag stays set) so we don't fight its temporary-enable. if self._pending_plugin_reconcile and not self.on_demand_active: - self._pending_plugin_reconcile = False - self._reconcile_enabled_plugins() + # Only clear the flag on success -- a retryable failure + # (e.g. discovery) leaves it set so the request isn't lost. + if self._reconcile_enabled_plugins(): + self._pending_plugin_reconcile = False if not self.available_modes: - # Nothing to render yet; keep servicing plugin update - # schedules and the reconcile flag so we pick up a mode - # as soon as one is enabled via the web UI, instead of - # having exited already at startup. - self._sleep_with_plugin_updates(30) + # Nothing to render yet. Re-check _pending_plugin_reconcile + # every ~1s (rather than a long sleep) so enabling a plugin + # via the web UI is picked up about as promptly as it would + # be once modes exist and the loop is iterating per-frame. + self._sleep_with_plugin_updates(1) continue # Handle on-demand commands before rendering @@ -2560,16 +2562,20 @@ def _unregister_plugin(self, plugin_id: str) -> None: self.plugin_modes.pop(mode, None) self.mode_to_plugin_id.pop(mode, None) - # Unsubscribe the plugin's config-change callback. Pop only after the - # unsubscribe attempt so a failed unsubscribe doesn't lose our only - # reference to the callback before config_service has processed it. + # Unsubscribe the plugin's config-change callback. Pop only on a + # successful unsubscribe -- if it raises, keep our reference so a + # later retry (or at least cleanup) still has the real callback + # instead of a lost one. callback = self._plugin_config_callbacks.get(plugin_id) if callback is not None and hasattr(self, 'config_service'): try: self.config_service.unsubscribe(callback, plugin_id=plugin_id) except Exception as e: logger.debug("Error unsubscribing plugin %s from config changes: %s", plugin_id, e) - self._plugin_config_callbacks.pop(plugin_id, None) + else: + self._plugin_config_callbacks.pop(plugin_id, None) + else: + self._plugin_config_callbacks.pop(plugin_id, None) self._plugin_accepts_display_mode.pop(plugin_id, None) @@ -2594,12 +2600,16 @@ def enabled_map(cfg: Dict[str, Any]) -> Dict[str, bool]: } return enabled_map(old_config) != enabled_map(new_config) - def _reconcile_enabled_plugins(self) -> None: + def _reconcile_enabled_plugins(self) -> bool: """Load/unload plugins so the running set matches the enabled set in config. Runs on the main display thread (never the config-watcher - thread) so mutating available_modes is race-free against rendering.""" + thread) so mutating available_modes is race-free against rendering. + + Returns True if reconciliation completed (including a no-op), or + False on a retryable failure -- the caller keeps the pending-reconcile + flag set in that case so the request isn't silently dropped.""" if self.plugin_manager is None: - return + return True try: config = self.config_service.get_config() except Exception as e: @@ -2609,7 +2619,14 @@ def _reconcile_enabled_plugins(self) -> None: discovered = set(self.plugin_manager.discover_plugins()) except Exception as e: logger.error("Plugin reconcile: discovery failed: %s", e, exc_info=True) - return + return False + + for p in discovered: + if p in config and not isinstance(config.get(p), dict): + logger.warning( + "Plugin reconcile: config for %s is a %s, not a dict; treating as disabled", + p, type(config.get(p)).__name__ + ) desired = { p for p in discovered @@ -2619,7 +2636,7 @@ def _reconcile_enabled_plugins(self) -> None: to_add = desired - current to_remove = current - desired if not to_add and not to_remove: - return + return True previous_mode = self.current_display_mode @@ -2639,6 +2656,7 @@ def _reconcile_enabled_plugins(self) -> None: self._resync_mode_index_after_change(previous_mode) logger.info("Plugin reconcile complete: +%s -%s (%d modes)", sorted(to_add), sorted(to_remove), len(self.available_modes)) + return True def _resync_mode_index_after_change(self, previous_mode: Optional[str]) -> None: """Clamp rotation state after available_modes changed. Stays on the diff --git a/test/test_display_controller_plugin_toggle.py b/test/test_display_controller_plugin_toggle.py index 856c0450..0980e729 100644 --- a/test/test_display_controller_plugin_toggle.py +++ b/test/test_display_controller_plugin_toggle.py @@ -131,19 +131,72 @@ def test_noop_when_enabled_set_unchanged(self, test_display_controller): assert controller.plugin_manager.load_plugin.call_count == load_calls assert controller.plugin_manager.unload_plugin.call_count == unload_calls - def test_reconcile_ignores_non_dict_config_value(self, test_display_controller): + def test_reconcile_ignores_non_dict_config_value(self, test_display_controller, caplog): """A malformed config value (e.g. a stray string where a plugin's section should be a dict) must be treated as disabled, not crash - the reconcile with AttributeError.""" + the reconcile with AttributeError, and should be logged so it's + visible to whoever has to debug the malformed config.""" controller = test_display_controller plugin = _make_plugin(["foo"]) _wire_plugin_manager(controller, {"foo": plugin}, discovered=["foo"]) _set_config(controller, {"foo": "not-a-dict"}) - controller._reconcile_enabled_plugins() # must not raise + with caplog.at_level("WARNING"): + controller._reconcile_enabled_plugins() # must not raise assert "foo" not in controller.plugin_display_modes assert "foo" not in controller.available_modes + assert any("foo" in r.message and "not a dict" in r.message for r in caplog.records) + + def test_disable_keeps_callback_when_unsubscribe_fails(self, test_display_controller): + """If config_service.unsubscribe() raises, _unregister_plugin must + keep the callback in _plugin_config_callbacks rather than losing the + only reference to it (it still tears down the plugin itself).""" + controller = test_display_controller + plugin = _make_plugin(["live"]) + _wire_plugin_manager(controller, {"sports": plugin}, discovered=["sports"]) + + _set_config(controller, {"sports": {"enabled": True}}) + controller._reconcile_enabled_plugins() + assert "sports" in controller._plugin_config_callbacks + + controller.config_service.unsubscribe = MagicMock(side_effect=RuntimeError("boom")) + + _set_config(controller, {"sports": {"enabled": False}}) + controller._reconcile_enabled_plugins() + + assert "sports" not in controller.plugin_display_modes + assert "sports" in controller._plugin_config_callbacks + + +class TestReconcileReturnValue: + """_reconcile_enabled_plugins() returns True/False so the caller (run()'s + loop) only clears _pending_plugin_reconcile on success, keeping a + retryable failure's request alive instead of silently dropping it.""" + + def test_returns_true_on_success(self, test_display_controller): + controller = test_display_controller + plugin = _make_plugin(["foo"]) + _wire_plugin_manager(controller, {"foo": plugin}, discovered=["foo"]) + _set_config(controller, {"foo": {"enabled": True}}) + assert controller._reconcile_enabled_plugins() is True + + def test_returns_true_for_noop(self, test_display_controller): + controller = test_display_controller + _wire_plugin_manager(controller, {}, discovered=[]) + _set_config(controller, {}) + assert controller._reconcile_enabled_plugins() is True + + def test_returns_false_on_discovery_failure(self, test_display_controller): + controller = test_display_controller + controller.plugin_manager.discover_plugins.side_effect = RuntimeError("boom") + _set_config(controller, {}) + assert controller._reconcile_enabled_plugins() is False + + def test_returns_true_when_no_plugin_manager(self, test_display_controller): + controller = test_display_controller + controller.plugin_manager = None + assert controller._reconcile_enabled_plugins() is True class TestRunWithNoModesEnabled: @@ -170,8 +223,11 @@ def fake_sleep(duration, tick_interval=1.0): controller.run() # Old behavior returned before ever reaching the loop body, so - # _sleep_with_plugin_updates would never have been called. - assert sleep_calls == [30, 30, 30] + # _sleep_with_plugin_updates would never have been called. The idle + # tick is short (not a long sleep) so a plugin enabled via the web + # UI while idle is picked up about as promptly as it would be once + # modes exist and the loop is iterating per-frame. + assert sleep_calls == [1, 1, 1] class TestEnabledSetChanged: