fix(display): retry a plugin that is enabled but failed to load (#495)

* fix(display): retry a plugin that is enabled but failed to load A plugin whose validate_config() returns False is treated as a hard load failure. The API then reports enabled=true, loaded=false, error=null: the plugin is simply absent, with nothing saying why. hockey-scoreboard sat in that state on a live rig for four days. The recovery path existed but could not be reached. _reconcile_enabled_plugins computes to_add = desired - current, and a plugin that failed to load is never in current, so it stays in to_add and would be retried. But the reconcile is queued by _enabled_set_changed(), which compares only top-level `enabled` flags -- and the edit that actually fixes such a plugin (enabling a league, filling in an API key) is nested inside the plugin's own config section. No top-level flag changes, so no reconcile is queued, and the save that should have fixed it does nothing. Only toggling some unrelated plugin -- which does change a top-level flag -- queues the global reconcile that recovers it. Add a second gate: queue a reconcile when a discovered plugin is enabled in config but absent from the running set. It is deliberately narrow rather than "reconcile on any config change". Reconcile calls discover_plugins(), a ~39-manifest filesystem scan, and it runs on the render thread; doing that on every config save would trade this bug for a frame hitch. Gating on plugin_manifests also keeps non-plugin sections that carry their own `enabled` flag (schedule, display) from queueing a reconcile they can never satisfy. In the steady state -- every enabled plugin loaded -- the new check is False and costs nothing. The same valid-but-unconfigured => hard-fail shape still exists in text-display, youtube-stats, birdnet-go, ledmatrix-flights and mqtt-notifications; this makes all of them recoverable without a restart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(display): snapshot the plugin mappings under their locks Addresses the review finding on the cross-thread reads. _enabled_plugin_not_running runs on the config-watcher thread and read two mappings the render thread mutates. Catching RuntimeError was not a fix: it turned a torn read into a coin flip between an unnecessary discovery scan and a missed retry, which is the bug this PR exists to remove. Both reads are now snapshots taken under the lock that guards their writes: - plugin_manifests via a new PluginManager.discovered_plugin_ids(), which copies the ids while holding the existing _discovery_lock. Discovery rebuilds that mapping entry by entry, so an unsynchronised reader can see it half-populated. - plugin_display_modes under a new controller lock, taken at the only two sites that mutate it (_register_loaded_plugin / _unregister_plugin). The locks are never nested -- each snapshot is taken and released before the next -- so this cannot deadlock against discovery, which holds _discovery_lock while it rebuilds. No cost on the per-frame path. Both mutation sites run during reconcile, which is rare, and every hot-path read of plugin_display_modes is on the render thread itself, same thread as the writes, so those stay lock-free. Tests: the accessor returns a snapshot rather than a live view, and actually takes the discovery lock (proved from a second thread, since an RLock is reentrant on the owning one) so a later refactor cannot quietly drop it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW * fix(display): consume the reconcile request before serving it Addresses the second review finding: a lost update on _pending_plugin_reconcile. The flag was cleared after a successful reconcile. Reconcile has already read its config by that point, so a config change arriving mid-flight set a flag that the trailing clear then erased -- a request that was never served, and the newest config never reconciled. That is the same "my save did nothing" symptom this PR exists to remove, so leaving it would have undercut the fix. Consume the request before running it instead, and re-arm only on a retryable failure. A change that lands during reconcile now stays set and is picked up on the next pass. The per-frame read stays lock-free. It is a fast path that can only produce a false negative -- the watcher setting the flag just after it is read is seen on the next iteration -- never a false positive that loses a request. The lock is taken only when a reconcile is actually pending or a config change arrives. Extracted _service_pending_reconcile() so the sequence is testable rather than buried in run()'s loop; the review asked for a regression test that invokes the subscriber during reconciliation, which is not reachable otherwise. Tests: 4 new, covering a request racing in mid-reconcile, the quiet success, the retryable-failure re-arm, and not reconciling when nothing is pending. Two of them fail against the previous clear-after-success semantics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-23 19:38:13 +00:00 · 2026-08-23 11:43:39 -04:00
parent 5a1f121e6b
commit c321b94085
4 changed files with 338 additions and 8 deletions
@@ -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 == []
@@ -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()