You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Removes ledmatrix-weather and music credential stubs from config_secrets.template.json
Clears the matching weather key from the inline fallback block in first_time_install.sh
Root cause
config_manager deep-merges config_secrets.json into the main config on every load. Because the secrets template shipped ledmatrix-weather and music as top-level keys, they appeared as plugin config entries in the merged config. The state reconciler treated them as plugins referenced in config but not installed on disk, and auto-downloaded and installed both on the first web UI visit after a fresh install.
Test plan
Fresh install from one-shot-install.sh — confirm only starlark-apps, web-ui-info appear in plugin-repos/ after boot
Open web UI plugin page — confirm neither ledmatrix-weather nor ledmatrix-music are auto-installed
Manually install ledmatrix-weather or ledmatrix-music via plugin store — confirm credentials can still be added to config_secrets.json and are picked up correctly
Enhanced plugin system reliability with improved error handling during updates, including automatic state recovery and comprehensive failure tracking to help plugins recover from temporary issues more gracefully.
Chores
Simplified first-time setup process by streamlining configuration initialization and removing unused credential template sections.
When execute_update() fails (timeout or unhandled exception), the plugin
state was set to ERROR with no recovery path. can_execute() returns False
for ERROR state, so the plugin's update() was never called again, leaving
it showing stale data indefinitely.
Instead, update plugin_last_update so the plugin waits one configured
interval before retrying, and keep the state ENABLED so recovery is
automatic on the next cycle.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…context
- Use time.time() at the point of failure instead of reusing current_time
(captured before execution), so the full retry interval always elapses
after a timeout rather than one execution-duration shorter
- Add PluginStateManager.set_error_info() to persist structured error context
without changing plugin state; call it in both failure branches so
get_error_info() / get_state_info() surface recoverable errors alongside
ERROR-state errors
- Add warning log on the success=False branch (was previously silent)
- Pass a descriptive Exception (not a generic "Plugin execution failed") to
health_tracker.record_failure() in the timeout/executor-error path
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The two-step set_state() / set_error_info() sequence left a window where
readers could observe ENABLED state without the accompanying error context.
Add threading.RLock to PluginStateManager and a new set_state_with_error()
method that holds the lock for both the state-transition write and the
_error_info write together. The method inlines the state-transition logic
rather than calling set_state() internally to intentionally skip the
"clear _error_info for non-ERROR states" side effect — the recoverable
error dict is exactly what we want stored.
Replace both paired set_state / set_error_info call sites in
run_scheduled_updates() with the single atomic method.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three verified issues:
- set_error_info wrote _error_info without holding _lock and stored the
caller's dict by reference, allowing races and post-write mutation
- set_state_with_error stored error_info by reference (lock was already held)
- get_error_info read _error_info without _lock and returned the live
reference, letting callers mutate the stored snapshot
Implicit fourth fix: set_state also wrote _error_info without _lock; locking
get_error_info while leaving that writer unguarded would have created a new
race, so set_state is now wrapped in _lock too for consistency.
Changes:
- set_state: wrap entire body in self._lock (covers _states, _state_history,
and _error_info writes atomically; ERROR-path _error_info value was already
a fresh dict literal so no copy needed)
- set_error_info: acquire self._lock + store dict(error_info) shallow copy
- set_state_with_error: store dict(error_info) shallow copy (lock already held)
- get_error_info: acquire self._lock + return dict(info) copy or None
All stored values are flat dicts of strings/floats/bools, so shallow copy
is sufficient — deepcopy is not needed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…act helper
update_all_plugins still set PluginState.ERROR on both failure paths, leaving
it inconsistent with the run_scheduled_updates fix from the same PR.
Extract _record_update_failure(plugin_id, exc=None) to hold all shared failure
logic: capture actual failure time, build structured error_info, log the retry
warning, stamp plugin_last_update, call set_state_with_error(ENABLED), and
forward to health_tracker. Replace all four failure sites (two in
run_scheduled_updates, two in update_all_plugins) with calls to this helper.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…template
config_secrets.template.json shipped ledmatrix-weather and music as
top-level keys; config_manager deep-merges secrets into the main config
on load, so the reconciler treated them as plugin config entries and
auto-installed both plugins on first web UI visit after a fresh install.
Remove both keys from the template and clear the inline fallback block
in first_time_install.sh so new installs start clean.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
🔍 Trigger review
📝 Walkthrough
Walkthrough
Removes credential sections from the configuration template and updates the initialization script to write empty JSON. Introduces a standardized failure-recovery mechanism in the plugin system that records update failures with structured error context while keeping plugins in ENABLED state, rather than transitioning them to permanent ERROR state.
Removes ledmatrix-weather and music credential sections from template; updates script to write empty {} object instead of template when config is missing.
Plugin State Management src/plugin_system/plugin_state.py
Adds re-entrant lock for thread-safety; introduces set_error_info() to store structured error context independently, and set_state_with_error() to atomically transition state while recording both transition history and error details; modifies get_error_info() to return shallow copy and become lock-guarded.
Introduces _record_update_failure() helper that stamps failure time, transitions plugin back to ENABLED state with structured error context via set_state_with_error(), logs retry warning, and records failure to health tracker; refactors run_scheduled_updates() and update_all_plugins() to use new recovery path instead of permanent ERROR state.
Sequence Diagram
sequenceDiagram
participant PM as Plugin Manager
participant PSM as Plugin State Manager
participant HT as Health Tracker
PM->>PM: Attempt plugin update
PM->>PM: Update fails (exception)
PM->>PSM: set_state_with_error(ENABLED, error_info)
activate PSM
PSM->>PSM: Acquire re-entrant lock
PSM->>PSM: Record error context
PSM->>PSM: Update state to ENABLED
PSM->>PSM: Add state transition to history
PSM->>PSM: Release lock
deactivate PSM
PM->>PM: Stamp plugin_last_update with failure time
PM->>HT: Record failure event
PM->>PM: Log retry warning
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The pull request title clearly and accurately summarizes the main change: preventing weather and music from auto-installing on fresh install by removing credential stubs from the template and fallback config.
Docstring Coverage
✅ Passed
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches📝 Generate docstrings
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch fix/plugin-error-state-no-recovery
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.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
While this PR successfully addresses the goal of preventing auto-installation of specific plugins, it introduces significant, undocumented architectural changes to the plugin system. The failure recovery logic has been modified to keep plugins in an ENABLED state rather than transitioning to ERROR, and a thread-safety layer (RLock) has been partially implemented.
Codacy analysis shows the PR is up to standards, but the new complexity in plugin_manager.py and plugin_state.py is not fully covered by tests or properly documented. The most critical concerns are the incomplete thread-safety implementation and the risk of infinite retry loops or broken monitoring due to the lifecycle changes.
About this PR
The change in plugin failure behavior—where a plugin remains ENABLED rather than moving to an ERROR state—is a significant lifecycle modification. This may impact external dashboards or monitoring tools that rely on the ERROR state to flag health issues.
The PR title and description focus exclusively on installation fixes, but the code contains significant architectural changes to plugin error handling, retry logic, and thread-safety in PluginStateManager. These changes should be acknowledged and justified in the PR description to ensure reviewers and future maintainers understand the rationale.
1 comment outside of the diffsrc/plugin_system/plugin_manager.py
line 760 ⚪ LOW RISK
Suggestion: This dynamic class generation using type() inside a loop is unconventional and may interfere with the PluginExecutor if it performs introspection on class names or attributes. Consider using a standard wrapper class or passing monitoring logic directly.
Test suggestions
Verify config_secrets.json is generated without weather or music keys on fresh install.
Verify plugin update failures now transition to ENABLED state with error info (recoverable path) instead of ERROR state.
Verify thread-safety of PluginStateManager under concurrent state transitions and queries.
Verify the atomic 'set_state_with_error' method correctly persists both state and error context.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify plugin update failures now transition to ENABLED state with error info (recoverable path) instead of ERROR state.
2. Verify thread-safety of PluginStateManager under concurrent state transitions and queries.
3. Verify the atomic 'set_state_with_error' method correctly persists both state and error context.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Inconsistent timestamp type. The rest of the PluginStateManager uses datetime.now() for timestamps, but this dictionary uses a float from time.time(). Use datetime.now() to ensure consistency for UI/JSON serialization components.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The thread-safety implementation is incomplete. To avoid race conditions or inconsistent snapshots, all methods reading from or writing to internal state must acquire self._lock. Specifically, apply the lock to: record_update, record_display, get_state, is_loaded, is_enabled, is_running, is_error, can_execute, get_state_history, and get_state_info.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: This logic transitions failing plugins back to PluginState.ENABLED for automatic recovery instead of PluginState.ERROR. While this enables retries, ensure it does not lead to infinite retry loops or log spam for non-transient failures. Additionally, ensure that UI or monitoring components are updated to inspect get_error_info(), as a plugin can now be 'Enabled' while actively failing.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Suggestion: The logic for recording state transitions is duplicated across set_state and set_state_with_error. Refactor this into a private _record_transition method to improve maintainability.
update_all_plugins() still leaves recovered plugins marked unhealthy.
Failures on this path now call health_tracker.record_failure() through _record_update_failure() at Lines 715-716, but the success branch here never mirrors run_scheduled_updates() Lines 772-773 with a record_success(). A plugin can recover to ENABLED while the health tracker and any circuit-breaker logic stay stuck in failure mode.
Suggested fix
if success:
self.plugin_last_update[plugin_id] = time.time()
self.state_manager.record_update(plugin_id)
self.state_manager.set_state(plugin_id, PluginState.ENABLED)
+ if self.health_tracker:+ self.health_tracker.record_success(plugin_id)
else:
self._record_update_failure(plugin_id)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugin_system/plugin_manager.py` around lines 799 - 809, The success
branch in update_all_plugins sets plugin_last_update, calls
state_manager.record_update and sets state_manager.set_state(…,
PluginState.ENABLED) but never notifies the health tracker of recovery; mirror
the behavior in run_scheduled_updates by calling the health success path when
execute_update returns True (e.g., invoke
health_tracker.record_success(plugin_id) or the equivalent success-recording
method) right after state_manager.set_state so recovered plugins are removed
from failure/circuit-breaker state; leave the existing
_record_update_failure(exception) call in the except/false paths unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugin_system/plugin_manager.py`:
- Around line 706-710: The error_info dictionary is storing 'timestamp' as a
Unix float while PluginStateManager.set_state() uses a datetime for
PluginState.ERROR, causing inconsistent shapes; change the code that builds
error_info (the variable error_info in plugin_manager) to normalize failure_time
into the same datetime type used by PluginStateManager.set_state() (e.g.,
convert the float/ts to a datetime via datetime.fromtimestamp() or use
datetime.utcnow() at the failure site) so get_error_info()/get_state_info()
always return the same timestamp type; update any callers that construct
error_info to use the same datetime normalization and ensure serialization code
expects that unified format.
In `@src/plugin_system/plugin_state.py`:
- Around line 155-216: The docstring promises atomic writes in
set_state_with_error (which holds self._lock) but readers like get_state() and
get_state_info() read shared structures without locking, letting a reader see
the new state before _error_info is written; fix by making all readers that
access self._states, self._error_info or self._state_history (e.g., get_state(),
get_state_info(), any get_state_history/get_state_info variants) acquire
self._lock around their reads and return shallow copies as needed so callers
cannot mutate internal maps, ensuring the state+error atomicity guaranteed by
set_state_with_error.
---
Outside diff comments:
In `@src/plugin_system/plugin_manager.py`:
- Around line 799-809: The success branch in update_all_plugins sets
plugin_last_update, calls state_manager.record_update and sets
state_manager.set_state(…, PluginState.ENABLED) but never notifies the health
tracker of recovery; mirror the behavior in run_scheduled_updates by calling the
health success path when execute_update returns True (e.g., invoke
health_tracker.record_success(plugin_id) or the equivalent success-recording
method) right after state_manager.set_state so recovered plugins are removed
from failure/circuit-breaker state; leave the existing
_record_update_failure(exception) call in the except/false paths unchanged.
🪄 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: 0891d654-753d-4a35-b6d3-86848f8a4b39
📥 Commits
Reviewing files that changed from the base of the PR and between 65e3e83 and c70183f.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Normalize error_info["timestamp"] across failure paths.
This helper writes a Unix float, but PluginStateManager.set_state() still writes a datetime object for PluginState.ERROR in src/plugin_system/plugin_state.py Lines 76-79. get_error_info()/get_state_info() can now return two different shapes for the same field, which is easy to break in JSON serialization and UI formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugin_system/plugin_manager.py` around lines 706 - 710, The error_info
dictionary is storing 'timestamp' as a Unix float while
PluginStateManager.set_state() uses a datetime for PluginState.ERROR, causing
inconsistent shapes; change the code that builds error_info (the variable
error_info in plugin_manager) to normalize failure_time into the same datetime
type used by PluginStateManager.set_state() (e.g., convert the float/ts to a
datetime via datetime.fromtimestamp() or use datetime.utcnow() at the failure
site) so get_error_info()/get_state_info() always return the same timestamp
type; update any callers that construct error_info to use the same datetime
normalization and ensure serialization code expects that unified format.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, check if the files exist and examine the plugin_state.py implementation
find . -name "plugin_state.py" -type f
Repository: ChuckBuilds/LEDMatrix
Length of output: 101
🏁 Script executed:
# Get the full implementation of plugin_state.py to understand lock usage
cat -n src/plugin_system/plugin_state.py
Repository: ChuckBuilds/LEDMatrix
Length of output: 11280
🏁 Script executed:
# Check the plugin_manager.py file at the mentioned lines to see how get_state_info() is called
sed -n '560,570p' src/plugin_system/plugin_manager.py
sed -n '820,830p' src/plugin_system/plugin_manager.py
sed -n '710,720p' src/plugin_system/plugin_manager.py
Repository: ChuckBuilds/LEDMatrix
Length of output: 1363
🏁 Script executed:
# Verify the actual method implementations and lock usage one more time with clearer focus
rg -A 2 "def get_state\(" src/plugin_system/plugin_state.py
rg -A 15 "def get_state_info" src/plugin_system/plugin_state.py | head -20
Repository: ChuckBuilds/LEDMatrix
Length of output: 711
🏁 Script executed:
# Check what callers depend on get_state_info() atomicity
rg "get_state_info\|get_state\(" src/plugin_system/plugin_manager.py | head -20
Repository: ChuckBuilds/LEDMatrix
Length of output: 47
🏁 Script executed:
# Let me check plugin_manager.py line 714 to see the actual usage
sed -n '710,720p' src/plugin_system/plugin_manager.py
Repository: ChuckBuilds/LEDMatrix
Length of output: 627
🏁 Script executed:
# And confirm that get_state_info is called in places that would be affected
grep -n "get_state_info\|\.get_state(" src/plugin_system/plugin_manager.py | head -10
Repository: ChuckBuilds/LEDMatrix
Length of output: 350
Partial locking breaks the atomicity guarantee: readers can observe state changes without error context.
set_state_with_error() acquires _lock to write both _states and _error_info atomically, but get_state(), get_state_info(), and other readers do not acquire the lock. A concurrent caller can read the new state from _states (via unlocked get_state()) before _error_info is written, defeating the atomicity contract described in the docstring. This directly undermines the recoverable-failure recovery mechanism used at src/plugin_system/plugin_manager.py line 714.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugin_system/plugin_state.py` around lines 155 - 216, The docstring
promises atomic writes in set_state_with_error (which holds self._lock) but
readers like get_state() and get_state_info() read shared structures without
locking, letting a reader see the new state before _error_info is written; fix
by making all readers that access self._states, self._error_info or
self._state_history (e.g., get_state(), get_state_info(), any
get_state_history/get_state_info variants) acquire self._lock around their reads
and return shallow copies as needed so callers cannot mutate internal maps,
ensuring the state+error atomicity guaranteed by set_state_with_error.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
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
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.
Summary
ledmatrix-weatherandmusiccredential stubs fromconfig_secrets.template.jsonweatherkey from the inline fallback block infirst_time_install.shRoot cause
config_managerdeep-mergesconfig_secrets.jsoninto the main config on every load. Because the secrets template shippedledmatrix-weatherandmusicas top-level keys, they appeared as plugin config entries in the merged config. The state reconciler treated them as plugins referenced in config but not installed on disk, and auto-downloaded and installed both on the first web UI visit after a fresh install.Test plan
one-shot-install.sh— confirm onlystarlark-apps,web-ui-infoappear inplugin-repos/after bootledmatrix-weathernorledmatrix-musicare auto-installedledmatrix-weatherorledmatrix-musicvia plugin store — confirm credentials can still be added toconfig_secrets.jsonand are picked up correctly🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores