feat: activate dormant plugin health/metrics subsystem and surface it in the web UI#388
Merged
Merged
Conversation
PluginManager shipped a fully-built health tracker, resource monitor and
circuit breaker that were never instantiated (health_tracker/resource_monitor
were left as None), so the circuit breaker never engaged and the existing
health/metrics API routes always returned "not available".
- DisplayController now wires a PluginHealthTracker and PluginResourceMonitor
onto the plugin manager, enabling the circuit breaker (a repeatedly-failing
plugin's update() is skipped after consecutive failures, then retried after
a cooldown) and per-plugin execution-time metrics. Both persist to the
shared cache.
- load_plugin() now validates each plugin's config against its JSON schema in
a strictly warn/degrade-only way: a violation logs a warning and flags the
plugin degraded in the health tracker, but never changes whether the plugin
loads or its pass/fail behaviour. Adds PluginHealthTracker.set_degraded(),
which never touches the circuit breaker.
- ResourceMonitor CPU/memory sampling now reuses a cached psutil.Process and
reads cpu_percent(interval=None), so monitoring no longer blocks ~100ms per
call on the display loop's update path.
- Fix DiskCache.get() raising TypeError for max_age=None ("never expires"),
which silently discarded persisted plugin health/metrics on read and thus
broke cross-process and post-restart surfacing.
- Fix two dead PluginManager helpers that called non-existent tracker methods.
Tests: new test_resource_monitor, test_plugin_health,
test_plugin_manager_schema_soft; extended test_cache_manager and
test_display_controller.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvTav268UXv44ub9K11LYq
With the health/metrics subsystem now active in the display service, expose it in the web UI (which runs as a separate process from the display loop): - Wire a health tracker / resource monitor backed by the shared on-disk cache into the web process so /api/v3/plugins/health and /plugins/metrics read the data the display service persists. - Build those route responses per installed plugin id (the tracker's in-memory view is empty in a fresh web process) so cross-process data is included. - Add state + error_info to /plugins/installed entries so the UI can show why a plugin isn't running instead of just loaded:false. - Add a "Plugin Health" panel to the Tools page (circuit status, avg/max update time, update count, last error) plus PluginAPI.getPluginMetrics(). Tests: route-level tests for the health/metrics endpoints in test_web_api. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UvTav268UXv44ub9K11LYq
|
Warning Review limit reached
Next review available in: 40 minutes 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 (7)
📝 WalkthroughWalkthroughThis PR adds plugin health/degraded-state tracking, non-blocking soft JSON schema validation during plugin load, cached-process resource monitoring, and a DiskCache max_age=None fix. It wires health/metrics through DisplayController, web app startup, API v3 endpoints, the JS client, and a new Plugin Health UI panel. ChangesPlugin Health, Metrics, and Soft Schema Validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant PluginManager
participant SchemaManager
participant PluginHealthTracker
participant Cache
PluginManager->>PluginManager: load_plugin(plugin_id, config)
PluginManager->>PluginManager: validate_config()
PluginManager->>PluginManager: _validate_config_schema_soft(plugin_id, config)
PluginManager->>SchemaManager: validate_config_against_schema(config)
SchemaManager-->>PluginManager: validation result (valid/errors)
alt schema invalid
PluginManager->>PluginManager: _set_degraded_safe(plugin_id, reason)
PluginManager->>PluginHealthTracker: set_degraded(plugin_id, reason)
PluginHealthTracker->>Cache: persist degraded state
else schema valid
PluginManager->>PluginHealthTracker: set_degraded(plugin_id, None)
PluginHealthTracker->>Cache: clear degraded state
end
PluginManager-->>PluginManager: plugin load continues (pass/fail unchanged)
sequenceDiagram
participant Browser
participant ToolsUI as tools.html
participant APIClient as PluginAPI
participant APIv3 as api_v3 blueprint
participant HealthTracker as PluginHealthTracker
participant ResourceMonitor as PluginResourceMonitor
Browser->>ToolsUI: refreshPluginHealth(force)
ToolsUI->>APIClient: getPluginHealth()
APIClient->>APIv3: GET /api/v3/plugins/health
APIv3->>APIv3: _installed_plugin_ids()
APIv3->>HealthTracker: get_health_summary(pid) per plugin
HealthTracker-->>APIv3: health summaries
APIv3-->>APIClient: health data
ToolsUI->>APIClient: getPluginMetrics()
APIClient->>APIv3: GET /api/v3/plugins/metrics
APIv3->>ResourceMonitor: get_metrics_summary(pid) per plugin
ResourceMonitor-->>APIv3: metrics summaries
APIv3-->>APIClient: metrics data
APIClient-->>ToolsUI: merged health + metrics
ToolsUI->>Browser: render plugin-health-tbody rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 | 26 |
| 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.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
![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/cache/disk_cache.py (1)
71-77: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate type hint and docstring for
max_age=Nonesupport.The method now correctly handles
max_age=Noneat runtime, but the type hint still saysmax_age: int = 300while multiple callers passNone(e.g.PluginHealthTracker._load_health_state,PluginResourceMonitor.get_metrics). TheMemoryCache.getalready usesOptional[int]. The docstring should also document theNonesemantics.As per coding guidelines: "Use type hints for function parameters and return values" and "Use docstrings for classes and complex functions."
🔧 Proposed fix for type hint and docstring
- def get(self, key: str, max_age: int = 300) -> Optional[Dict[str, Any]]: + def get(self, key: str, max_age: Optional[int] = 300) -> Optional[Dict[str, Any]]: """ Get data from disk cache. Args: key: Cache key - max_age: Maximum age in seconds + max_age: Maximum age in seconds (None = never expires) Returns: Cached data or None if not found or expired """🤖 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/cache/disk_cache.py` around lines 71 - 77, The `DiskCache.get` signature and docstring need to match its `max_age=None` behavior. Update the `get` method in `DiskCache` to use an `Optional[int]` type hint for `max_age`, aligned with `MemoryCache.get`, and revise the docstring to state that `None` disables age-based expiration. Make sure the `get` method’s parameter docs clearly describe both the default integer case and the `None` semantics so callers like `PluginHealthTracker._load_health_state` and `PluginResourceMonitor.get_metrics` are reflected correctly.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 `@web_interface/blueprints/api_v3.py`:
- Around line 2230-2243: The per-plugin health/metrics polling is using cached
summaries that can stay stale because the tracker is a long-lived singleton and
the read path only loads persisted data once. Update the summary read flow in
api_v3’s `/plugins/health` and `/plugins/metrics` handling to add a
refresh/bypass option or explicitly invalidate the per-plugin cache before
calling get_health_summary() and get_metrics_summary(). Use the existing tracker
methods on health_tracker/metrics_tracker so the display process always reflects
the latest values instead of the first snapshot.
---
Outside diff comments:
In `@src/cache/disk_cache.py`:
- Around line 71-77: The `DiskCache.get` signature and docstring need to match
its `max_age=None` behavior. Update the `get` method in `DiskCache` to use an
`Optional[int]` type hint for `max_age`, aligned with `MemoryCache.get`, and
revise the docstring to state that `None` disables age-based expiration. Make
sure the `get` method’s parameter docs clearly describe both the default integer
case and the `None` semantics so callers like
`PluginHealthTracker._load_health_state` and `PluginResourceMonitor.get_metrics`
are reflected correctly.
🪄 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: 66a521db-39f6-4f25-a159-6360a9cc83e1
📒 Files selected for processing (15)
src/cache/disk_cache.pysrc/display_controller.pysrc/plugin_system/plugin_health.pysrc/plugin_system/plugin_manager.pysrc/plugin_system/resource_monitor.pytest/test_cache_manager.pytest/test_display_controller.pytest/test_plugin_health.pytest/test_plugin_manager_schema_soft.pytest/test_resource_monitor.pytest/test_web_api.pyweb_interface/app.pyweb_interface/blueprints/api_v3.pyweb_interface/static/v3/js/plugins/api_client.jsweb_interface/templates/v3/partials/tools.html
… hints Addresses CodeRabbit review on #388: - Major: the web process's health/resource trackers cached the first persisted read in an in-memory dict (and the CacheManager memory tier held max_age=None entries indefinitely), so a long-lived web process showed the first snapshot and never reflected the display service's later updates. Add an opt-in force_reload path (get_health_summary/get_health_state/_load_health_state and get_metrics_summary/get_metrics) that bypasses the in-memory copy and, via a new memory_ttl passthrough on CacheManager.get, the cache manager's memory tier — so each /plugins/health and /plugins/metrics poll reads fresh persisted state. Default behaviour (force_reload=False) is unchanged for the display process and existing callers. - Minor: DiskCache.get type hint is now Optional[int] with the None ("never expires") semantics documented, matching MemoryCache.get. Tests: new force_reload staleness cases in test_plugin_health and test_resource_monitor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UvTav268UXv44ub9K11LYq
ChuckBuilds
added a commit
that referenced
this pull request
Jul 11, 2026
…update (#392) run_scheduled_updates()'s resource-monitor branch wrapped the update call in a closure stored as a *class* attribute on a dynamically-built type (type('obj', (object,), {'update': monitored_update})()). The descriptor protocol turns a function found via class-attribute lookup into a bound method on instance access, silently prepending the synthetic instance as an implicit first argument -- but monitored_update() takes none, so every call raised "monitored_update() takes 0 positional arguments but 1 was given", was caught by run_scheduled_updates' try/except, and recorded as an update failure. self.resource_monitor is None by default and was dormant until PR #388 ("activate dormant plugin health/metrics subsystem") wired it up in both display_controller.py and web_interface/app.py -- meaning this bug went live in every real deployment as of that merge (2026-07-09) despite the buggy line itself dating back to 2025-12-27. In practice this means no plugin's update() has succeeded since upgrading past #388: circuit breakers cycle through half-open -> immediate failure -> reopened every health-check interval forever, and all plugin data (scores, odds, prices, etc.) goes stale from whatever was last fetched before the upgrade. Confirmed live on a running instance: odds-ticker (and stock-news, ledmatrix-stocks, baseball-scoreboard, ledmatrix-leaderboard, of-the-day) failing this exact way every 5-minute circuit-breaker retry. Fixed by using types.SimpleNamespace(update=monitored_update) instead of a dynamic class: SimpleNamespace stores attributes on the instance itself, so attribute lookup returns the plain function unchanged -- never routed through the class-attribute descriptor protocol that injects an implicit self. Added test_run_scheduled_updates_calls_update_with_resource_monitor to test/test_plugin_system.py using a real PluginResourceMonitor (not a mock of it), so the test exercises the actual descriptor-binding behavior that caused this. Verified the test fails with the exact reported error against the pre-fix code and passes against the fix.
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
2 participants
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.
Pull Request
Summary
Lights up the plugin health/metrics infrastructure that already existed in core but was never wired up:
PluginManager.health_trackerand.resource_monitorwere left asNone, so the circuit breaker never engaged and the health/metrics API routes always returned "not available". This is Phase 1 of the project-wide reliability plan — turning on and surfacing what was already built, plus the small fixes needed to make it work end-to-end.Type of change
Related issues
Refs the core-reliability improvement plan (Phase 1). No tracked issue.
Test plan
EMULATOR=true python3 run.py)scripts/dev_server.py)pytest)Details:
test_resource_monitor,test_plugin_health,test_plugin_manager_schema_soft,test_cache_manager(disk-cachemax_age=Noneregression),test_display_controller(wiring), and route tests intest_web_apifor/plugins/health+/plugins/metrics.test_web_api::TestConfigAPI::test_save_double_sided_*failures are unrelated to this change (they fail identically onmainin this environment) and are left untouched.Documentation
Plugin compatibility
Schema validation added to
load_plugin()is warn/degrade-only: a config that violates a plugin's JSON schema logs a warning and flags the plugin degraded in the health tracker, but never changes whether the plugin loads or its pass/fail behaviour. No currently-loading plugin changes behaviour. The circuit breaker is now active, which is a deliberate reliability improvement (a plugin whoseupdate()fails repeatedly is paused and retried after a cooldown, rather than hammered every interval).Notes for reviewer
What changed
Backend (
feat(plugin-system)):DisplayControllerwires realPluginHealthTracker+PluginResourceMonitoronto the plugin manager → activates the circuit breaker and per-plugin execution-time metrics.load_plugin()runs warn/degrade-only schema validation via newPluginHealthTracker.set_degraded()(which never touches the circuit breaker).ResourceMonitornow reuses a cachedpsutil.Processand readscpu_percent(interval=None)— the oldinterval=0.1blocked ~100 ms per monitored call, unacceptable on the display loop.DiskCache.get()raisedTypeErrorformax_age=None("never expires") and silently treated the record as a miss — this had been quietly breaking persisted health/metrics reads across processes and restarts.PluginManagerhelpers that called non-existent tracker methods.Web (
feat(web-ui)):/plugins/healthand/plugins/metricsroutes build results per installed plugin id so they read the cross-process persisted data./plugins/installedentries now includestate+error_info.PluginAPI.getPluginMetrics().Scope decisions worth a second opinion
DiskCachemax_age=Nonefix, which was a prerequisite.monitor_call()persists to the disk cache on every call, so routing the ~30 fpsdisplay()path through it would cause excessive disk I/O. Update-time monitoring (this PR) captures the main cost; display-time monitoring can be added later with sampling/batching.🤖 Generated with Claude Code
https://claude.ai/code/session_01UvTav268UXv44ub9K11LYq
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores