feat(display-controller): hot-reload plugin enable/disable without a restart (#374)

* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: ChuckBuilds <charlesmynard@gmail.com>
2026-08-01 08:48:05 +00:00 · 2026-07-06 17:37:01 -04:00
parent bea00448d3
commit 7a6bad29fe
2 changed files with 466 additions and 47 deletions
@@ -0,0 +1,255 @@
+"""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
+
+    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, 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"})
+
+        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:
+    """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. 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:
+    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