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

src/display_controller.py

@@ -181,6 +181,16 @@ 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]] = {}
+ # 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 @@ def _controller_config_change(old_config: Dict[str, Any], new_config: Dict[str,
  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 @@ def run(self):
  # 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 @@ def _register_loaded_plugin(self, plugin_id: str) -> List[str]:
  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 @@ def config_change_callback(old_config: Dict[str, Any], new_config: Dict[str, Any
  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 @@ def enabled_map(cfg: Dict[str, Any]) -> Dict[str, bool]:
  }
  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

src/plugin_system/plugin_manager.py

@@ -631,6 +631,17 @@ def reload_plugin(self, plugin_id: str) -> bool:
 
  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.

test/test_display_controller_plugin_toggle.py

@@ -6,6 +6,7 @@
 the dispatch maps on the main thread when the enabled set changes.
 """
 
+import copy
 from unittest.mock import MagicMock
 
 
@@ -253,3 +254,182 @@ def test_ignores_non_enabled_value_edits(self, test_display_controller):
  {"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 == []