diff --git a/src/display_controller.py b/src/display_controller.py index e739e08e..bb650698 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -181,6 +181,16 @@ class DisplayController: 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]] = {} + # plugin_display_modes is mutated only by _register_loaded_plugin / + # _unregister_plugin on the render thread, but the config-watcher + # thread reads it in _enabled_plugin_not_running. Both mutation sites + # run during reconcile (rare), so this lock never touches the per-frame + # path -- the hot-path reads are same-thread as the writes. + self._plugin_modes_lock = threading.Lock() + # Guards the consume-and-clear of _pending_plugin_reconcile. Only taken + # when a reconcile is actually pending or a config change arrives, both + # rare -- the per-frame path just reads the bool. + self._reconcile_flag_lock = threading.Lock() # Per-plugin config-change callbacks, kept so we can unsubscribe a # plugin when it is disabled live. self._plugin_config_callbacks: Dict[str, Callable] = {} @@ -463,8 +473,10 @@ class DisplayController: 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 + if (self._enabled_set_changed(old_config, new_config) + or self._enabled_plugin_not_running(new_config)): + with self._reconcile_flag_lock: + self._pending_plugin_reconcile = True self.config_service.subscribe(_controller_config_change) @@ -1749,11 +1761,12 @@ class DisplayController: # 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. + # The lock-free read is a fast path only; it can be a false + # negative (the watcher setting the flag just after it is read + # is seen next iteration), never a false positive that loses a + # request. if self._pending_plugin_reconcile and not self.on_demand_active: - # 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 + self._service_pending_reconcile() if not self.available_modes: # Nothing to render yet. Re-check _pending_plugin_reconcile @@ -2813,7 +2826,8 @@ class DisplayController: 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) + with self._plugin_modes_lock: + 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 @@ -2847,7 +2861,8 @@ class DisplayController: 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, []) + with self._plugin_modes_lock: + modes = self.plugin_display_modes.pop(plugin_id, []) for mode in modes: if mode in self.available_modes: self.available_modes.remove(mode) @@ -2892,6 +2907,67 @@ class DisplayController: } return enabled_map(old_config) != enabled_map(new_config) + def _service_pending_reconcile(self) -> None: + """Consume a pending reconcile request and run it. + + The request is consumed BEFORE reconciling, not cleared after. Clearing + after would drop any config change that lands while reconcile is + running: reconcile has already read its config by then, so the clear + erases a request it never served and the newest config never + reconciles -- the same "your save did nothing" failure this whole path + exists to prevent. Consuming first means such a request stays set and + is picked up on the next pass. + + A retryable failure (e.g. discovery) re-arms the flag. + """ + with self._reconcile_flag_lock: + pending = self._pending_plugin_reconcile + self._pending_plugin_reconcile = False + if pending and not self._reconcile_enabled_plugins(): + with self._reconcile_flag_lock: + self._pending_plugin_reconcile = True + + def _enabled_plugin_not_running(self, new_config: Dict[str, Any]) -> bool: + """True when a discovered plugin is enabled in config but not running. + + ``_enabled_set_changed`` compares only top-level ``enabled`` flags, which + misses the case that strands a plugin: one whose ``validate_config()`` + returned False is absent from the running set, and the edit that fixes it + (enabling a league, filling in an API key) lives *nested* inside that + plugin's own section. No top-level flag changes, so no reconcile is + queued, and the save that should have fixed it appears to do nothing -- + only toggling some unrelated plugin recovers it. hockey-scoreboard sat + enabled-but-absent on a live rig for four days this way. + + Deliberately narrow: it fires only for ids the plugin manager has + actually discovered, so non-plugin sections that carry their own + ``enabled`` flag (``schedule``, ``display``, ...) don't queue a reconcile + on every save. In the steady state -- everything enabled is loaded -- + this is False and costs nothing. That matters because reconcile runs + ``discover_plugins()`` on the render thread, where a needless + filesystem scan per config save would show up as a frame hitch. + + Runs on the config-watcher thread, so both mappings it reads are + snapshotted under the lock that guards their writes. + """ + if self.plugin_manager is None: + return False + # Two snapshots, each taken under its own lock and never nested, so a + # half-written mapping is never observed and this can't deadlock + # against discovery (which holds the discovery lock while rebuilding). + try: + known = self.plugin_manager.discovered_plugin_ids() + except AttributeError: + # Older manager without the accessor: fall back to a plain read. + known = set(getattr(self.plugin_manager, 'plugin_manifests', ()) or ()) + with self._plugin_modes_lock: + running = set(self.plugin_display_modes) + for key, value in new_config.items(): + if (key in known and isinstance(value, dict) + and value.get('enabled', False) and key not in running): + return True + return False + 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 diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 543017c9..0dc7a428 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -631,6 +631,17 @@ class PluginManager: return self.load_plugin(plugin_id) + def discovered_plugin_ids(self) -> set: + """Snapshot of the discovered plugin ids, taken under the discovery lock. + + Callers on other threads (the config watcher) must not iterate + ``plugin_manifests`` directly: discovery rebuilds it entry by entry, so + an unsynchronised reader can see a half-populated mapping or raise + "dictionary changed size during iteration". + """ + with self._discovery_lock: + return set(self.plugin_manifests) + def get_plugin(self, plugin_id: str) -> Optional[Any]: """ Get a loaded plugin instance by ID. diff --git a/test/test_display_controller_plugin_toggle.py b/test/test_display_controller_plugin_toggle.py index 0980e729..e09933d0 100644 --- a/test/test_display_controller_plugin_toggle.py +++ b/test/test_display_controller_plugin_toggle.py @@ -6,6 +6,7 @@ These tests cover the reconcile path that loads/unloads plugins and rebuilds the dispatch maps on the main thread when the enabled set changes. """ +import copy from unittest.mock import MagicMock @@ -253,3 +254,182 @@ class TestEnabledSetChanged: {"a": {"enabled": True, "duration": 30}}, {"a": {"enabled": True, "duration": 45}}, ) is False + + +class TestEnabledPluginNotRunning: + """A plugin that fails validate_config() is enabled but absent, and the + config edit that fixes it is nested inside the plugin's own section -- so + the top-level ``enabled`` comparison never sees it. These cover the second + gate that queues a reconcile in that case. + """ + + def test_nested_edit_is_invisible_to_the_enabled_set_check(self, test_display_controller): + """The original gate: proves why a second one is needed.""" + controller = test_display_controller + old = {"hockey-scoreboard": {"enabled": True, "nhl": {"enabled": False}}} + new = {"hockey-scoreboard": {"enabled": True, "nhl": {"enabled": True}}} + # Enabling a league changes no top-level flag. + assert controller._enabled_set_changed(old, new) is False + + def test_queues_reconcile_when_enabled_plugin_is_absent(self, test_display_controller): + controller = test_display_controller + controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}} + controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"} + controller.plugin_display_modes = {} # failed to load + cfg = {"hockey-scoreboard": {"enabled": True, "nhl": {"enabled": True}}} + assert controller._enabled_plugin_not_running(cfg) is True + + def test_quiet_when_every_enabled_plugin_is_running(self, test_display_controller): + controller = test_display_controller + controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}} + controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"} + controller.plugin_display_modes = {"hockey-scoreboard": ["nhl"]} + cfg = {"hockey-scoreboard": {"enabled": True}} + assert controller._enabled_plugin_not_running(cfg) is False + + def test_disabled_plugin_does_not_queue(self, test_display_controller): + controller = test_display_controller + controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}} + controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"} + controller.plugin_display_modes = {} + cfg = {"hockey-scoreboard": {"enabled": False}} + assert controller._enabled_plugin_not_running(cfg) is False + + def test_non_plugin_sections_do_not_queue(self, test_display_controller): + """``schedule``/``display`` carry their own ``enabled`` and are never + in plugin_display_modes -- without the manifest check they would queue + a reconcile, and therefore a filesystem scan, on every config save.""" + controller = test_display_controller + controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}} + controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"} + controller.plugin_display_modes = {"hockey-scoreboard": ["nhl"]} + cfg = { + "hockey-scoreboard": {"enabled": True}, + "schedule": {"enabled": True}, + "display": {"enabled": True}, + } + assert controller._enabled_plugin_not_running(cfg) is False + + def test_non_dict_section_is_ignored(self, test_display_controller): + controller = test_display_controller + controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}} + controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"} + controller.plugin_display_modes = {} + assert controller._enabled_plugin_not_running({"hockey-scoreboard": "nonsense"}) is False + + def test_no_plugin_manager_is_quiet(self, test_display_controller): + controller = test_display_controller + controller.plugin_manager = None + assert controller._enabled_plugin_not_running({"x": {"enabled": True}}) is False + + +class TestReconcileQueuedThroughSubscriber: + """End-to-end through the real config-change subscriber, not the helper. + + Without the second gate this is the four-day-outage path: the plugin is + enabled, absent, and the save that enables its league sets no flag. + """ + + @staticmethod + def _subscriber(controller): + subs = controller.config_service._subscribers['*'] + for cb in subs: + if getattr(cb, '__name__', '') == '_controller_config_change': + return cb + raise AssertionError(f"controller subscriber not found among {subs}") + + @staticmethod + def _configs(controller, plugin_section_old, plugin_section_new): + """Build two full configs differing only inside the plugin section -- + the subscriber refreshes its cache from these, so they must be real.""" + base = copy.deepcopy(controller.config) + old = copy.deepcopy(base) + new = copy.deepcopy(base) + old["hockey-scoreboard"] = plugin_section_old + new["hockey-scoreboard"] = plugin_section_new + return old, new + + def test_nested_edit_queues_reconcile_for_absent_plugin(self, test_display_controller): + controller = test_display_controller + controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}} + controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"} + controller.plugin_display_modes = {} # validate_config() said False + controller._pending_plugin_reconcile = False + + old, new = self._configs( + controller, + {"enabled": True, "nhl": {"enabled": False}}, + {"enabled": True, "nhl": {"enabled": True}}, + ) + # The original gate is blind to this edit ... + assert controller._enabled_set_changed(old, new) is False + self._subscriber(controller)(old, new) + # ... but the reconcile is queued anyway. + assert controller._pending_plugin_reconcile is True + + def test_steady_state_does_not_queue_reconcile(self, test_display_controller): + """Everything enabled is running: an unrelated edit must not queue a + reconcile, or every config save drags a filesystem scan onto the + render thread.""" + controller = test_display_controller + controller.plugin_manager.plugin_manifests = {"hockey-scoreboard": {}} + controller.plugin_manager.discovered_plugin_ids.return_value = {"hockey-scoreboard"} + controller.plugin_display_modes = {"hockey-scoreboard": ["nhl"]} + controller._pending_plugin_reconcile = False + + old, new = self._configs( + controller, + {"enabled": True, "scroll_speed": 1}, + {"enabled": True, "scroll_speed": 2}, + ) + self._subscriber(controller)(old, new) + + assert controller._pending_plugin_reconcile is False + + +class TestPendingReconcileNotLost: + """A config change arriving *during* reconcile must not be discarded. + + The flag used to be cleared after a successful reconcile. Reconcile has + already read its config by then, so that clear erased a request it never + served and the newest config never reconciled -- the same "my save did + nothing" symptom this path exists to prevent. + """ + + def test_request_arriving_during_reconcile_survives(self, test_display_controller): + controller = test_display_controller + controller._pending_plugin_reconcile = True + + def reconcile_and_race(): + # The watcher thread queues another change while we are mid-flight. + with controller._reconcile_flag_lock: + controller._pending_plugin_reconcile = True + return True + + controller._reconcile_enabled_plugins = reconcile_and_race + controller._service_pending_reconcile() + + assert controller._pending_plugin_reconcile is True, \ + "a config change landing during reconcile was discarded" + + def test_flag_cleared_on_a_quiet_success(self, test_display_controller): + controller = test_display_controller + controller._pending_plugin_reconcile = True + controller._reconcile_enabled_plugins = lambda: True + controller._service_pending_reconcile() + assert controller._pending_plugin_reconcile is False + + def test_retryable_failure_rearms(self, test_display_controller): + controller = test_display_controller + controller._pending_plugin_reconcile = True + controller._reconcile_enabled_plugins = lambda: False + controller._service_pending_reconcile() + assert controller._pending_plugin_reconcile is True + + def test_no_reconcile_when_nothing_pending(self, test_display_controller): + controller = test_display_controller + controller._pending_plugin_reconcile = False + calls = [] + controller._reconcile_enabled_plugins = lambda: calls.append(1) or True + controller._service_pending_reconcile() + assert calls == [] diff --git a/test/test_plugin_manager_discovered_ids.py b/test/test_plugin_manager_discovered_ids.py new file mode 100644 index 00000000..5bedba8d --- /dev/null +++ b/test/test_plugin_manager_discovered_ids.py @@ -0,0 +1,63 @@ +"""Tests for PluginManager.discovered_plugin_ids(). + +The config-watcher thread needs the set of discovered plugin ids while the +render thread may be rebuilding plugin_manifests. Iterating that dict directly +can observe a half-populated mapping or raise "dictionary changed size during +iteration", so the accessor snapshots it under the discovery lock. +""" + +import tempfile +import threading +from pathlib import Path + +import pytest + +from src.plugin_system.plugin_manager import PluginManager + + +@pytest.fixture +def pm(): + with tempfile.TemporaryDirectory() as tmp: + yield PluginManager(plugins_dir=str(Path(tmp) / "plugins")) + + +def test_returns_the_discovered_ids(pm): + pm.plugin_manifests = {"clock-simple": {}, "hockey-scoreboard": {}} + assert pm.discovered_plugin_ids() == {"clock-simple", "hockey-scoreboard"} + + +def test_empty_when_nothing_discovered(pm): + pm.plugin_manifests = {} + assert pm.discovered_plugin_ids() == set() + + +def test_is_a_snapshot_not_a_live_view(pm): + """The caller iterates the result on another thread; it must not alias + the mapping discovery is still writing to.""" + pm.plugin_manifests = {"clock-simple": {}} + snapshot = pm.discovered_plugin_ids() + pm.plugin_manifests["hockey-scoreboard"] = {} + assert snapshot == {"clock-simple"} + + +def test_takes_the_discovery_lock(pm): + """Guards against the lock being dropped in a later refactor: with the + lock held by another thread the call must block rather than read.""" + pm.plugin_manifests = {"clock-simple": {}} + finished = threading.Event() + + def call(): + pm.discovered_plugin_ids() + finished.set() + + pm._discovery_lock.acquire() + try: + # RLock is reentrant per-thread, so use a *different* thread to prove + # the accessor actually waits on it. + t = threading.Thread(target=call, daemon=True) + t.start() + assert not finished.wait(timeout=0.3), "accessor did not take the discovery lock" + finally: + pm._discovery_lock.release() + t.join(timeout=2) + assert finished.is_set()