Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions config/config_secrets.template.json
Original file line number Diff line number Diff line change
@@ -1,17 +1,9 @@
{
"ledmatrix-weather": {
"api_key": "YOUR_OPENWEATHERMAP_API_KEY"
},
"youtube": {
"api_key": "YOUR_YOUTUBE_API_KEY",
"channel_id": "YOUR_YOUTUBE_CHANNEL_ID"
},
"music": {
"SPOTIFY_CLIENT_ID": "YOUR_SPOTIFY_CLIENT_ID_HERE",
"SPOTIFY_CLIENT_SECRET": "YOUR_SPOTIFY_CLIENT_SECRET_HERE",
"SPOTIFY_REDIRECT_URI": "http://127.0.0.1:8888/callback"
},
"github": {
"api_token": "YOUR_GITHUB_PERSONAL_ACCESS_TOKEN"
}
}
}
6 changes: 1 addition & 5 deletions first_time_install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -598,11 +598,7 @@ if [ ! -f "$PROJECT_ROOT_DIR/config/config_secrets.json" ]; then
else
echo "⚠ Template config/config_secrets.template.json not found; creating a minimal secrets file"
cat > "$PROJECT_ROOT_DIR/config/config_secrets.json" <<'EOF'
{
"weather": {
"api_key": "YOUR_OPENWEATHERMAP_API_KEY"
}
}
{}
EOF
# Check if service runs as root and set ownership accordingly
SERVICE_USER="root"
Expand Down
54 changes: 42 additions & 12 deletions src/plugin_system/plugin_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,44 @@ def _get_plugin_update_interval(self, plugin_id: str, plugin_instance: Any) -> O
# Default: 60 seconds
return 60.0

def _record_update_failure(
self,
plugin_id: str,
exc: Optional[Exception] = None,
) -> None:
"""Apply the standard failure-recovery path for a plugin update.

Stamps plugin_last_update with the actual failure time so the full
configured interval elapses before the next retry, then transitions
the plugin back to ENABLED (not ERROR) with structured error context
so automatic recovery happens on the next scheduled cycle.

Args:
plugin_id: Plugin identifier
exc: The exception that caused the failure, if any. When None a
synthetic ExecutionFailure exception is constructed from the
timeout/executor-error path.
"""
failure_time = time.time()
if exc is not None:
err: Exception = exc
error_type = type(exc).__name__
else:
err = Exception(f"Plugin {plugin_id} execution failed (timeout or executor error)")
error_type = 'ExecutionFailure'

error_info = {
'error': str(err),
'error_type': error_type,
'timestamp': failure_time,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.

'recoverable': True,
Comment on lines +706 to +710

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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.

}
self.logger.warning("Plugin %s update() failed; will retry after interval", plugin_id)
self.plugin_last_update[plugin_id] = failure_time
self.state_manager.set_state_with_error(plugin_id, PluginState.ENABLED, error_info, error=err)
if self.health_tracker:
self.health_tracker.record_failure(plugin_id, err)
Comment on lines +714 to +716

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.


def run_scheduled_updates(self, current_time: Optional[float] = None) -> None:
"""
Trigger plugin updates based on their defined update intervals.
Expand Down Expand Up @@ -734,16 +772,10 @@ def monitored_update():
if self.health_tracker:
self.health_tracker.record_success(plugin_id)
else:
# Execution failed (timeout or error)
self.state_manager.set_state(plugin_id, PluginState.ERROR)
if self.health_tracker:
self.health_tracker.record_failure(plugin_id, Exception("Plugin execution failed"))
self._record_update_failure(plugin_id)
except Exception as exc: # pylint: disable=broad-except
self.logger.exception("Error updating plugin %s: %s", plugin_id, exc)
self.state_manager.set_state(plugin_id, PluginState.ERROR, error=exc)
# Record failure
if self.health_tracker:
self.health_tracker.record_failure(plugin_id, exc)
self._record_update_failure(plugin_id, exc=exc)

def update_all_plugins(self) -> None:
"""
Expand All @@ -769,14 +801,12 @@ def update_all_plugins(self) -> None:
if success:
self.plugin_last_update[plugin_id] = time.time()
self.state_manager.record_update(plugin_id)
# Update state back to ENABLED
self.state_manager.set_state(plugin_id, PluginState.ENABLED)
else:
# Execution failed
self.state_manager.set_state(plugin_id, PluginState.ERROR)
self._record_update_failure(plugin_id)
except Exception as exc: # pylint: disable=broad-except
self.logger.exception("Error updating plugin %s: %s", plugin_id, exc)
self.state_manager.set_state(plugin_id, PluginState.ERROR, error=exc)
self._record_update_failure(plugin_id, exc=exc)

def get_plugin_health_metrics(self) -> Dict[str, Any]:
"""
Expand Down
141 changes: 104 additions & 37 deletions src/plugin_system/plugin_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
with state transitions and queries.
"""

import threading
from enum import Enum
from typing import Optional, Dict, Any
from datetime import datetime
Expand Down Expand Up @@ -34,6 +35,7 @@ def __init__(self, logger: Optional[logging.Logger] = None) -> None:
logger: Optional logger instance
"""
self.logger = logger or get_logger(__name__)
self._lock = threading.RLock()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.

See Complexity in Codacy

self._states: Dict[str, PluginState] = {}
self._state_history: Dict[str, list] = {}
self._error_info: Dict[str, Dict[str, Any]] = {}
Expand All @@ -48,44 +50,44 @@ def set_state(
) -> None:
"""
Set plugin state and record transition.

Args:
plugin_id: Plugin identifier
state: New state
error: Optional error if transitioning to ERROR state
"""
old_state = self._states.get(plugin_id, PluginState.UNLOADED)
self._states[plugin_id] = state

# Record state transition
if plugin_id not in self._state_history:
self._state_history[plugin_id] = []

transition = {
'timestamp': datetime.now(),
'from': old_state.value,
'to': state.value,
'error': str(error) if error else None
}
self._state_history[plugin_id].append(transition)

# Store error info if transitioning to ERROR state
if state == PluginState.ERROR and error:
self._error_info[plugin_id] = {
'error': str(error),
'error_type': type(error).__name__,
'timestamp': datetime.now()
with self._lock:
old_state = self._states.get(plugin_id, PluginState.UNLOADED)
self._states[plugin_id] = state

if plugin_id not in self._state_history:
self._state_history[plugin_id] = []

transition = {
'timestamp': datetime.now(),
'from': old_state.value,
'to': state.value,
'error': str(error) if error else None
}
elif state != PluginState.ERROR:
# Clear error info when leaving ERROR state
self._error_info.pop(plugin_id, None)

self.logger.debug(
"Plugin %s state transition: %s → %s",
plugin_id,
old_state.value,
state.value
)
self._state_history[plugin_id].append(transition)

# Store error info if transitioning to ERROR state
if state == PluginState.ERROR and error:
self._error_info[plugin_id] = {
'error': str(error),
'error_type': type(error).__name__,
'timestamp': datetime.now()
}
elif state != PluginState.ERROR:
# Clear error info when leaving ERROR state
self._error_info.pop(plugin_id, None)

self.logger.debug(
"Plugin %s state transition: %s → %s",
plugin_id,
old_state.value,
state.value
)

def get_state(self, plugin_id: str) -> PluginState:
"""
Expand Down Expand Up @@ -136,17 +138,82 @@ def get_state_history(self, plugin_id: str) -> list:
"""
return self._state_history.get(plugin_id, [])

def set_error_info(self, plugin_id: str, error_info: Dict[str, Any]) -> None:
"""
Persist structured error context without changing plugin state.

Used for recoverable failures (e.g. update timeout) where the plugin
stays ENABLED but the error details should remain queryable.

Args:
plugin_id: Plugin identifier
error_info: Arbitrary dict describing the error
"""
with self._lock:
self._error_info[plugin_id] = dict(error_info)

def set_state_with_error(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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.

self,
plugin_id: str,
state: PluginState,
error_info: Dict[str, Any],
error: Optional[Exception] = None,
) -> None:
"""Set plugin state and persist error context atomically.

Unlike calling set_state() then set_error_info() separately, this
method holds ``_lock`` for both writes so no reader can observe the
new state without the accompanying error context.

Intentionally does not clear ``_error_info`` the way set_state() does
for non-ERROR transitions — this is the recoverable-failure path where
the error dict is the entire point.

Args:
plugin_id: Plugin identifier
state: New state
error_info: Structured error dict to persist alongside the state
error: Optional exception recorded in the transition history
"""
with self._lock:
old_state = self._states.get(plugin_id, PluginState.UNLOADED)
self._states[plugin_id] = state

if plugin_id not in self._state_history:
self._state_history[plugin_id] = []
self._state_history[plugin_id].append({
'timestamp': datetime.now(),
'from': old_state.value,
'to': state.value,
'error': str(error) if error else None,
})

self._error_info[plugin_id] = dict(error_info)

self.logger.debug(
"Plugin %s state transition: %s → %s (recoverable error stored)",
plugin_id,
old_state.value,
state.value,
)

def get_error_info(self, plugin_id: str) -> Optional[Dict[str, Any]]:
"""
Get error information for a plugin in ERROR state.

Get error information for a plugin.

Returns the stored error dict whether the plugin is in ERROR state or
still ENABLED after a recoverable failure. Returns a shallow copy so
callers cannot mutate the stored snapshot.

Args:
plugin_id: Plugin identifier

Returns:
Error information dict or None
Copy of the error information dict, or None
"""
return self._error_info.get(plugin_id)
with self._lock:
info = self._error_info.get(plugin_id)
return dict(info) if info is not None else None
Comment on lines +155 to +216

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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.


def record_update(self, plugin_id: str) -> None:
"""Record that plugin update() was called."""
Expand Down