feat(display-controller): hot-reload plugin enable/disable without a restart#374
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>
|
Warning Review limit reached
Next review available in: 57 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesLive Plugin Hot-Reload Lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant config_service
participant DisplayController
participant run_loop as run()
participant plugin_manager
participant _unregister_plugin
participant _register_loaded_plugin
participant _resync_mode_index_after_change
config_service->>DisplayController: enabled set changed
DisplayController->>DisplayController: _pending_plugin_reconcile = True
run_loop->>plugin_manager: discover_plugins()
run_loop->>_unregister_plugin: disable removed plugin_id
_unregister_plugin->>plugin_manager: unload_plugin(plugin_id)
run_loop->>plugin_manager: load_plugin(plugin_id)
run_loop->>_register_loaded_plugin: register loaded plugin_id
run_loop->>_resync_mode_index_after_change: previous_mode
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 | 35 |
| 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
…otation 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
There was a problem hiding this comment.
Actionable comments posted: 2
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)
1565-1567: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the reconcile flag set until reconciliation succeeds.
Line 1566 clears
_pending_plugin_reconcilebefore_reconcile_enabled_plugins()runs, but reconciliation can return early on discovery failure at Line 2612. A transient discovery/config issue then drops the enable/disable change until another config edit occurs. Have_reconcile_enabled_plugins()return success/failure, or re-set the flag on retryable failures.Also applies to: 2608-2612
🤖 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 1565 - 1567, The pending plugin reconcile flag is being cleared in DisplayController before reconciliation actually succeeds, so a discovery/config failure in _reconcile_enabled_plugins() can lose the requested enable/disable change. Update the flow around the self._pending_plugin_reconcile check to keep the flag set until _reconcile_enabled_plugins() reports success, or have _reconcile_enabled_plugins() return a success/failure result and only clear the flag on success. Also handle the early-return path in _reconcile_enabled_plugins() so retryable failures leave the flag set for a later retry.
🧹 Nitpick comments (1)
test/test_display_controller_plugin_toggle.py (1)
134-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider asserting the malformed-config warning is logged.
Test confirms no crash and the plugin is excluded from
plugin_display_modes/available_modes, but doesn't verify that a warning is actually logged for the malformed section — useful given the emphasis on comprehensive logging and clear troubleshooting messages for remote Raspberry Pi debugging.✅ Optional enhancement using caplog
def test_reconcile_ignores_non_dict_config_value(self, test_display_controller): """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.""" controller = test_display_controller plugin = _make_plugin(["foo"]) _wire_plugin_manager(controller, {"foo": plugin}, discovered=["foo"]) _set_config(controller, {"foo": "not-a-dict"}) - controller._reconcile_enabled_plugins() # must not raise + controller._reconcile_enabled_plugins() # must not raise assert "foo" not in controller.plugin_display_modes assert "foo" not in controller.available_modes + # Optionally, if caplog fixture is added as a param: + # assert any("foo" in r.message for r in caplog.records)As per coding guidelines, "Implement comprehensive logging for remote debugging on Raspberry Pi" and "Provide clear error messages for troubleshooting."
🤖 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 `@test/test_display_controller_plugin_toggle.py` around lines 134 - 147, The test currently verifies that _reconcile_enabled_plugins ignores a non-dict config value, but it does not assert that the malformed-config warning is emitted. Update test_reconcile_ignores_non_dict_config_value to capture logs (for example with caplog) and assert that the warning from _reconcile_enabled_plugins or the related logging path is present when "foo" is set to a non-dict value, while keeping the existing no-crash and disabled-plugin assertions.Source: Coding guidelines
🤖 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`:
- Around line 1569-1575: The idle path in DisplayController is using a fixed
30-second sleep that can delay plugin hot-enable because it does not react
promptly to _pending_plugin_reconcile. Update the no-available-modes branch in
the display loop to either use a much shorter tick or make
_sleep_with_plugin_updates respect the reconcile flag so it can wake early when
a plugin is enabled. Keep the behavior aligned with the existing
reconcile/plugin update handling in DisplayController.
- Around line 2563-2572: In the callback cleanup path in display_controller’s
unsubscribe logic, the callback is currently removed from
_plugin_config_callbacks even when config_service.unsubscribe fails, which loses
the only reference needed for retry or later cleanup. Move the pop so it only
happens after a successful unsubscribe, and keep the callback stored when
unsubscribe raises; also narrow the broad Exception in this block to the
specific exception type(s) raised by config_service.unsubscribe if possible.
---
Outside diff comments:
In `@src/display_controller.py`:
- Around line 1565-1567: The pending plugin reconcile flag is being cleared in
DisplayController before reconciliation actually succeeds, so a discovery/config
failure in _reconcile_enabled_plugins() can lose the requested enable/disable
change. Update the flow around the self._pending_plugin_reconcile check to keep
the flag set until _reconcile_enabled_plugins() reports success, or have
_reconcile_enabled_plugins() return a success/failure result and only clear the
flag on success. Also handle the early-return path in
_reconcile_enabled_plugins() so retryable failures leave the flag set for a
later retry.
---
Nitpick comments:
In `@test/test_display_controller_plugin_toggle.py`:
- Around line 134-147: The test currently verifies that
_reconcile_enabled_plugins ignores a non-dict config value, but it does not
assert that the malformed-config warning is emitted. Update
test_reconcile_ignores_non_dict_config_value to capture logs (for example with
caplog) and assert that the warning from _reconcile_enabled_plugins or the
related logging path is present when "foo" is set to a non-dict value, while
keeping the existing no-crash and disabled-plugin assertions.
🪄 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: 07bfb023-e45a-43de-9469-a20f46e19839
📒 Files selected for processing (2)
src/display_controller.pytest/test_display_controller_plugin_toggle.py
- 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
2 participants
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