feat(display-controller): hot-reload plugin enable/disable without a restart#374
Open
rpierce99 wants to merge 1 commit into
Open
feat(display-controller): hot-reload plugin enable/disable without a restart#374rpierce99 wants to merge 1 commit into
rpierce99 wants to merge 1 commit into
Conversation
…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>
📝 WalkthroughWalkthrough
ChangesLive Plugin Hot-Reload Lifecycle
Sequence Diagram(s)sequenceDiagram
participant config_service
participant DisplayController
participant _reconcile_enabled_plugins
participant plugin_manager
participant _unregister_plugin
participant _register_loaded_plugin
participant _resync_mode_index_after_change
config_service->>DisplayController: config_changed callback (enabled set differs)
DisplayController->>DisplayController: _pending_plugin_reconcile = True
DisplayController->>_reconcile_enabled_plugins: run() loop detects flag
_reconcile_enabled_plugins->>plugin_manager: discover_plugins()
_reconcile_enabled_plugins->>_unregister_plugin: each newly disabled plugin_id
_unregister_plugin->>plugin_manager: unload_plugin(plugin_id)
_reconcile_enabled_plugins->>plugin_manager: load_plugin(plugin_id) for each newly enabled
plugin_manager-->>_reconcile_enabled_plugins: loaded instance
_reconcile_enabled_plugins->>_register_loaded_plugin: plugin_id
_register_loaded_plugin->>config_service: subscribe(per-plugin callback)
_reconcile_enabled_plugins->>_resync_mode_index_after_change: previous_mode
_resync_mode_index_after_change-->>DisplayController: current_mode_index clamped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 30 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/display_controller.py (1)
1547-1566:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKeep the controller alive when the mode list becomes empty.
Line 1547 still exits on startup with zero modes, so a later live-enable cannot be observed. Also, after Line 1566 disables the last plugin, the loop continues with
available_modes == []and later hits modulo-by-zero during rotation. Add an idle/no-modes path that waits for the watcher to set_pending_plugin_reconcileinstead of exiting/crashing.As per coding guidelines, implement graceful degradation and provide clear troubleshooting behavior when configuration is missing or invalid.
🐛 Proposed fix
- if not self.available_modes: - logger.warning("No display modes are enabled. Exiting.") - self.display_manager.cleanup() - return + if not self.available_modes: + logger.warning("No display modes are enabled; idling until configuration enables a plugin.") @@ if self._pending_plugin_reconcile and not self.on_demand_active: self._pending_plugin_reconcile = False self._reconcile_enabled_plugins() + + if not self.available_modes and not self.on_demand_active: + self.current_mode_index = 0 + self.current_display_mode = None + try: + self.display_manager.clear() + self.display_manager.update_display() + except (OSError, RuntimeError, ValueError) as err: + logger.debug( + "Unable to clear display while no plugin modes are enabled: %s", + err, + exc_info=True, + ) + self._sleep_with_plugin_updates(1.0) + continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/display_controller.py` around lines 1547 - 1566, The early exit condition that checks `if not self.available_modes` at the start of the method causes the controller to exit immediately if no modes are available at startup, preventing later live-enables from being observed. Additionally, when the last plugin is disabled after Line 1566, the loop continues with an empty available_modes list and crashes during mode rotation due to modulo-by-zero. Remove the early return statement that exits when no modes are available, and instead add a graceful idle path inside the main while loop that detects when available_modes becomes empty and waits for the watcher to set _pending_plugin_reconcile flag, triggering _reconcile_enabled_plugins() to restore modes without exiting or crashing.Source: Coding guidelines
🧹 Nitpick comments (2)
src/display_controller.py (2)
2554-2560: ⚡ Quick winDo not lose the callback handle before unsubscribe succeeds.
Line 2555 pops the callback first; if Line 2558 fails,
ConfigServicecan still hold a callback that captures the disabled plugin instance, but the controller no longer has the handle to retry removal. Keep the entry until unsubscribe succeeds, and narrow the exception type instead of swallowingException.As per coding guidelines, catch specific exceptions and clean up resources regularly to manage memory effectively.
♻️ Proposed refactor
- callback = self._plugin_config_callbacks.pop(plugin_id, None) + 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) + except (KeyError, ValueError, RuntimeError) as e: + logger.warning( + "Error unsubscribing plugin %s from config changes: %s", + plugin_id, + e, + exc_info=True, + ) + else: + self._plugin_config_callbacks.pop(plugin_id, None) + else: + self._plugin_config_callbacks.pop(plugin_id, None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/display_controller.py` around lines 2554 - 2560, In the plugin callback unsubscription logic, the callback is being popped from _plugin_config_callbacks before the unsubscribe call succeeds. This means if the config_service.unsubscribe call fails, the callback handle is lost but the ConfigService still holds a reference to the callback capturing the disabled plugin instance, causing memory leaks. Move the pop operation to occur only after a successful unsubscribe call, and replace the broad Exception catch with a more specific exception type that reflects what the unsubscribe method actually raises. This ensures that if unsubscribe fails, you retain the handle for potential retry logic and prevent resource leaks.Sources: Coding guidelines, Linters/SAST tools
2591-2594: ⚡ Quick winLog and narrow the live-config fallback.
Line 2593 silently catches every exception and falls back to stale
self.config, which makes plugin toggles appear ignored with no remote-debugging clue. Catch the expected config-read failure types and log the fallback.As per coding guidelines, catch specific exceptions and implement comprehensive logging for remote debugging on Raspberry Pi.
♻️ Proposed refactor
try: config = self.config_service.get_config() - except Exception: + except (OSError, RuntimeError, ValueError) as err: + logger.warning( + "Plugin reconcile: using cached config because live config read failed: %s", + err, + exc_info=True, + ) config = self.config🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/display_controller.py` around lines 2591 - 2594, In the try-except block where self.config_service.get_config() is called, replace the bare except Exception clause with specific exception types that represent expected config-read failures and add logging when the fallback to stale self.config occurs. This will provide visibility into why the fallback happened for remote debugging purposes instead of silently ignoring all exceptions.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/display_controller.py`:
- Line 2601: The dict comprehension building the desired set assumes each
discovered plugin's config section is a dictionary. If a malformed hot-reloaded
config contains non-dict values like null or strings for a plugin entry (e.g.,
"sports": null), calling .get() on it will raise AttributeError and crash the
controller. Validate that config.get(p) returns a dictionary before calling
.get('enabled', False) on it; if it's None or not a dict, treat it as a disabled
plugin with a sensible default value to handle missing or invalid configuration
gracefully.
---
Outside diff comments:
In `@src/display_controller.py`:
- Around line 1547-1566: The early exit condition that checks `if not
self.available_modes` at the start of the method causes the controller to exit
immediately if no modes are available at startup, preventing later live-enables
from being observed. Additionally, when the last plugin is disabled after Line
1566, the loop continues with an empty available_modes list and crashes during
mode rotation due to modulo-by-zero. Remove the early return statement that
exits when no modes are available, and instead add a graceful idle path inside
the main while loop that detects when available_modes becomes empty and waits
for the watcher to set _pending_plugin_reconcile flag, triggering
_reconcile_enabled_plugins() to restore modes without exiting or crashing.
---
Nitpick comments:
In `@src/display_controller.py`:
- Around line 2554-2560: In the plugin callback unsubscription logic, the
callback is being popped from _plugin_config_callbacks before the unsubscribe
call succeeds. This means if the config_service.unsubscribe call fails, the
callback handle is lost but the ConfigService still holds a reference to the
callback capturing the disabled plugin instance, causing memory leaks. Move the
pop operation to occur only after a successful unsubscribe call, and replace the
broad Exception catch with a more specific exception type that reflects what the
unsubscribe method actually raises. This ensures that if unsubscribe fails, you
retain the handle for potential retry logic and prevent resource leaks.
- Around line 2591-2594: In the try-except block where
self.config_service.get_config() is called, replace the bare except Exception
clause with specific exception types that represent expected config-read
failures and add logging when the fallback to stale self.config occurs. This
will provide visibility into why the fallback happened for remote debugging
purposes instead of silently ignoring all exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a018e79a-c0a2-41ca-b90b-b306870b29c6
📒 Files selected for processing (2)
src/display_controller.pytest/test_display_controller_plugin_toggle.py
| 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)} |
There was a problem hiding this comment.
Validate plugin config sections before calling .get().
Line 2601 assumes each discovered plugin section is a dict. A malformed hot-reloaded config like "sports": null or "sports": "disabled" raises AttributeError here and tears down the controller from the outer run-loop exception handler.
As per coding guidelines, validate inputs and handle errors early, check required fields and data types on startup/config changes, and provide fallback/default values when configuration is missing or invalid.
🐛 Proposed fix
- desired = {p for p in discovered if config.get(p, {}).get('enabled', False)}
+ desired = set()
+ for plugin_id in discovered:
+ plugin_config = config.get(plugin_id, {})
+ if plugin_config is None:
+ plugin_config = {}
+ if not isinstance(plugin_config, dict):
+ logger.warning(
+ "Plugin reconcile: config for %s must be an object; treating it as disabled",
+ plugin_id,
+ )
+ continue
+ if plugin_config.get('enabled', False):
+ desired.add(plugin_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/display_controller.py` at line 2601, The dict comprehension building the
desired set assumes each discovered plugin's config section is a dictionary. If
a malformed hot-reloaded config contains non-dict values like null or strings
for a plugin entry (e.g., "sports": null), calling .get() on it will raise
AttributeError and crash the controller. Validate that config.get(p) returns a
dictionary before calling .get('enabled', False) on it; if it's None or not a
dict, treat it as a disabled plugin with a sensible default value to handle
missing or invalid configuration gracefully.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Enabling or disabling a plugin in
config.jsonrequired restarting the display service. The core already hot-reloads plugin config values (a 2sConfigServicefile-watcher →BasePlugin.on_config_change), but the enabled set was restart-only:available_modes, the plugin instances and the mode→plugin dispatch maps are built once duringDisplayController.__init__, and the run loop never revisits them. So toggling a plugin in the web UI did nothing until a restart.Change
The controller now reconciles its running plugins against the config's enabled set whenever that set changes:
Watcher thread stays cheap and safe. The
ConfigServicesubscriber only diffs the enabled flags (_enabled_set_changed) and sets a_pending_plugin_reconcileflag. It never touches loop state.Reconcile runs on the render thread. At the top of each run-loop iteration (deferred while on-demand is active),
_reconcile_enabled_pluginsdiffs desired-vs-running plugins and:_unregister_plugin(remove modes, unsubscribe the plugin's config callback, thenplugin_manager.unload_plugin→cleanup()+on_disable()),current_mode_indexso the currently-displayed mode stays valid (stays put if it survived, otherwise restarts in range).Doing this on the render thread means mutating
available_modescan't race with rendering.Shared registration. The per-plugin registration done at startup is extracted into
_register_loaded_pluginand reused by the live-enable path, so startup and hot-enable build identical state.Incidental fix
Extracting the registration also fixes a latent late-binding bug: the per-plugin
on_config_changecallbacks were closures over theplugin_id/plugin_instanceloop variables, so after startup every plugin's callback referenced the last-loaded instance. Each callback now binds its own id/instance.Scope / notes
discover_plugins) is handled by the same path — it gets unloaded and removed from rotation.Tests
test/test_display_controller_plugin_toggle.py(uses the existingtest_display_controllerfixture with mocked managers + realConfigService):unload_plugin,current_mode_indexis clamped when the showing mode is removed,_enabled_set_changeddetects toggles / new sections and ignores non-enabledvalue edits.Full suite:
816 passed, 59 skipped, coverage 42.7% (gate 30%). Two failures intest/web_interface/test_state_reconciliation.pyare pre-existing onmain(reproduced with this change stashed) and unrelated to the display controller.Summary by CodeRabbit
New Features
Tests