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
When execute_update() returns False (timeout or unhandled exception), the plugin was set to PluginState.ERROR with no recovery mechanism — can_execute() returns False for ERROR state, permanently silencing the plugin's update() method
Instead, update plugin_last_update on failure so the plugin waits one configured interval before retrying, and keep state ENABLED so can_execute() returns True
Root cause (confirmed on devpi)
The weather plugin was showing 3-day-old forecast data. Investigation found:
Cache file timestamp: 2026-04-25 07:35 — 3 days old despite the service running continuously
Zero weather update log messages in 11 hours of journal
The weather plugin's _update_radar() fetches up to 12 RainViewer tiles sequentially (each timeout=10s, worst case 120s), exceeding the executor's default 30s timeout
On the second successful weather API call (when forecast_data gets lat/lon and radar fetching starts for the first time), the update timed out → ERROR state → permanent silence on all subsequent calls
This bug affects any plugin that ever times out on a slow network call — once it hits ERROR, it never recovers until a service restart.
Test plan
Deployed to devpi, restarted service — "Weather data updated for Tampa: 72.48°" appeared in logs within seconds, cache file updated with today's timestamp
Confirm that a plugin in a simulated timeout recovers on the next interval cycle
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>
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3df34a41-ec42-4bac-8206-bf16086ac11e
📥 Commits
Reviewing files that changed from the base of the PR and between 7b50a2d and 54b131c.
📒 Files selected for processing (1)
src/plugin_system/plugin_manager.py
📝 Walkthrough
Walkthrough
Centralizes recoverable update-failure handling: update failures (false return or exception) are timestamped, persisted as structured error_info, plugin state is kept ENABLED via set_state_with_error(...), and failures are recorded in the health tracker. Plugin state manager adds locking and APIs to persist error context atomically.
Adds _record_update_failure helper; refactors run_scheduled_updates / update_all_plugins to treat false returns and exceptions as recoverable: compute failure_time, build structured error_info, update plugin_last_update, call state_manager.set_state_with_error(..., PluginState.ENABLED, error_info, error=...), and call health_tracker.record_failure instead of setting PluginState.ERROR.
Plugin State Storage & Sync src/plugin_system/plugin_state.py
Adds a re-entrant lock to guard state, transition history, and error-info. Introduces set_error_info(...) and set_state_with_error(...). Refactors set_state(...) to perform writes under the lock and updates get_error_info(...) to return a lock-guarded shallow copy; documents that error info may persist while plugin remains ENABLED for recoverable failures.
Sequence Diagram(s)
sequenceDiagram
participant Scheduler as Scheduler / PluginManager
participant Plugin as Plugin
participant StateMgr as PluginStateManager
participant Health as HealthTracker
Scheduler->>Plugin: invoke update hook
alt success (True)
Plugin-->>Scheduler: returns True
Scheduler->>StateMgr: set_state(plugin_id, PluginState.ENABLED)
Scheduler->>Health: record_success(plugin_id)
else failure (False)
Plugin-->>Scheduler: returns False
Scheduler->>Scheduler: failure_time = time.time()
Scheduler->>StateMgr: set_state_with_error(plugin_id, PluginState.ENABLED, error_info, error=err)
Scheduler->>Health: record_failure(plugin_id, err)
else exception
Plugin-->>Scheduler: raises Exception
Scheduler->>Scheduler: failure_time = time.time()
Scheduler->>StateMgr: set_state_with_error(plugin_id, PluginState.ENABLED, error_info, error=exc)
Scheduler->>Health: record_failure(plugin_id, exc)
end
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The pull request title 'fix(plugin_manager): prevent permanent ERROR state after update timeout' directly and concisely describes the main change—preventing plugins from being permanently stuck in ERROR state after update timeouts, allowing automatic recovery.
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 correctly identifies the cause of permanent plugin lockout, the implementation is currently incomplete. Although Codacy analysis reports the code is 'Up to Standards', the fix is only applied to the periodic update path. The update_all_plugins method still transitions plugins to an ERROR state upon failure, which contradicts the PR's intent.
Furthermore, the current changes remove diagnostic metadata (the exception object) when setting the plugin state, which will make troubleshooting future failures more difficult. No automated tests were included to verify the recovery logic or prevent regressions.
About this PR
Incomplete scope: The update_all_plugins method (lines 752-781) still contains the logic that sets a plugin to PluginState.ERROR on failure. This execution path remains vulnerable to the bug described in the PR and should be updated to ensure system-wide resilience.
Lack of automated tests: No unit or integration tests were added to confirm the recovery behavior. Automated verification is necessary to ensure that the plugin_last_update timestamp is correctly refreshed and that plugins remain ENABLED across various failure modes.
Test suggestions
Verify plugin state remains ENABLED and last_update is refreshed when execute_update returns False (simulated timeout).
Verify plugin state remains ENABLED and last_update is refreshed when execute_update raises an Exception.
Verify the plugin successfully executes in a subsequent cycle after a previous failure (recovery check).
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify plugin state remains ENABLED and last_update is refreshed when execute_update returns False (simulated timeout).
2. Verify plugin state remains ENABLED and last_update is refreshed when execute_update raises an Exception.
3. Verify the plugin successfully executes in a subsequent cycle after a previous failure (recovery check).
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 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`:
- Line 740: The code sets self.plugin_last_update[plugin_id] = current_time
(captured before plugin execution) which can allow premature retries if
execution times out; update the failure and timeout branches to stamp the
last-update with the actual failure time by calling time.time() at the point of
failure/timeout instead of reusing current_time (locations referencing
plugin_last_update, plugin_id, and current_time).
- Around line 741-743: When re-enabling plugins after an update failure,
preserve structured error context before calling
state_manager.set_state(plugin_id, PluginState.ENABLED): capture the exception
details and persist them via the PluginStateManager API (e.g. use an error_info
field or a set_error_info(plugin_id, error_info) method) and continue to call
health_tracker.record_failure(plugin_id, err) with the full exception so
diagnostics remain available; update the block around PluginState.ENABLED (and
the analogous block at lines ~748–750) to first store the recoverable update
error into the plugin state or plugin info and add comprehensive logging, and
ensure plugin update() implementations surface API failures in a catch that
forwards structured error objects to state_manager and health_tracker.
🪄 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: 8d49a57c-ca33-4c35-a785-39de456615db
📥 Commits
Reviewing files that changed from the base of the PR and between 4ef3f8c and d0969ad.
…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 reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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 748-751: The two separate writes (state_manager.set_state(...) and
state_manager.set_error_info(...)) create a race where readers can see ENABLED
without error context; add an atomic API on PluginStateManager (e.g.,
set_state_with_error(plugin_id, state, error_info) or
set_recoverable_state(plugin_id, error_info)) that updates state and error_info
together under the same lock, then replace the paired calls in
run_scheduled_updates() (both the block at the current plugin_last_update
assignment and the similar block at the later location) to call this single
atomic method; alternatively, if you cannot change PluginStateManager, guard the
pair with the existing per-plugin lock used elsewhere in run_scheduled_updates()
so the two writes are performed while holding the same lock.
🪄 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: cacf5ccb-9422-4753-86ef-12c10dffa65a
📥 Commits
Reviewing files that changed from the base of the PR and between d0969ad and 37566d9.
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>
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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_state.py`:
- Around line 141-198: set_error_info currently writes to _error_info without
holding _lock and stores the caller's dict by reference, allowing races and
post-write mutation; make set_error_info acquire self._lock before mutating
self._error_info and store a defensive copy (e.g., shallow copy or deepcopy) of
error_info instead of the original reference, and update set_state_with_error to
also store a defensive copy (it already uses _lock) so both paths persist
immutable snapshots; also ensure any reader methods that access _error_info
(e.g., get_error_info or callers that read _error_info) either acquire the same
_lock or return their own copied snapshot to preserve atomicity for readers.
🪄 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: 7cbc9bf5-5827-45da-b807-afcc85129b38
📥 Commits
Reviewing files that changed from the base of the PR and between 37566d9 and 09c7940.
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>
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
execute_update()returnsFalse(timeout or unhandled exception), the plugin was set toPluginState.ERRORwith no recovery mechanism —can_execute()returnsFalseforERRORstate, permanently silencing the plugin'supdate()methodplugin_last_updateon failure so the plugin waits one configured interval before retrying, and keep stateENABLEDsocan_execute()returnsTrueRoot cause (confirmed on devpi)
The weather plugin was showing 3-day-old forecast data. Investigation found:
2026-04-25 07:35— 3 days old despite the service running continuously_update_radar()fetches up to 12 RainViewer tiles sequentially (eachtimeout=10s, worst case 120s), exceeding the executor's default 30s timeoutforecast_datagets lat/lon and radar fetching starts for the first time), the update timed out →ERRORstate → permanent silence on all subsequent callsThis bug affects any plugin that ever times out on a slow network call — once it hits
ERROR, it never recovers until a service restart.Test plan
"Weather data updated for Tampa: 72.48°"appeared in logs within seconds, cache file updated with today's timestamp🤖 Generated with Claude Code
Summary by CodeRabbit