mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 06:15:09 +00:00
* fix(store): don't read a ZIP-installed plugin's remote from the LEDMatrix repo update_plugin looked up remote.origin.url with `git -C <plugin> config --local` for plugins that are not git checkouts. Under plugin-repos/ git walks up to the enclosing LEDMatrix repository, so the lookup returned LEDMatrix's own URL and a plugin missing from the registry was "reinstalled" from the LEDMatrix repo. Only ask git when the plugin directory has its own .git, the test _get_local_git_info already uses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(schema): report each missing required field once, by name validate_config_against_schema ran its own required-fields loop after Draft7Validator.iter_errors, which already yields one `required` error per missing field, so every missing top-level field was listed twice. The validator's copy also printed the schema's whole `required` list ("Missing required property '['api_key', 'city']'") instead of the field. Drop the loop and take the field name from the error itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(store): stop mangling repository URLs that contain ".git" install_from_url and fetch_registry_from_url cleaned URLs with `rstrip('/').replace('.git', '')`, which removes ".git" anywhere: https://github.com/user/my.github.io became .../myhub.io, so installing or browsing that repository asked GitHub for one that does not exist. Add src/plugin_system/repo_urls.py with one anchored normalize_repo_url(), same_repo() for comparisons, github_owner_repo() and github_api_headers(), and use them for the five copies of the owner/repo parsing and GitHub headers in the store and for saved repositories. GitHub URLs are now recognised by urlparse().hostname everywhere: _get_latest_commit_info used a substring test, and _install_from_monorepo_api parsed any host. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(store): install a repository whose only branch is not main/master _install_via_git returned None both when every clone failed and when the last-resort clone of the repository's default branch succeeded. _install_plugin_impl papered over it with `and not plugin_path.exists()`; install_from_url did not, so a repository whose only branch is e.g. `develop` was cloned, then treated as a failure, then "downloaded" from main/master archives that do not exist. After a default-branch clone, return the branch the clone checked out (read from .git/HEAD), so None means failure and nothing else, and give both callers the same `branch_used is None` fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(plugins): judge the memory limit on each call's own growth monitor_call stores `metrics.memory_mb = max(previous, growth)`, and _check_limits compared that high-water mark with max_memory_mb. It never decreases, so once one update() grew the process past the limit every later call raised ResourceLimitExceeded and the circuit breaker kept reopening. Pass the call's own RSS growth to _check_limits; keep the high-water mark for reporting and document what it measures. Remove ResourceMetrics.update_average_execution_time: nothing called it, and it overwrote the running total with the average. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(plugins): reload_plugin re-reads the manifest from the discovered directory reload_plugin read `plugins_dir / plugin_id / "manifest.json"`, ignoring the discovery map and the plugin_dirs rules. For a plugin whose directory name differs from its manifest id the path did not exist, the re-read was skipped without a word, and the reload kept the stale manifest. Resolve the directory with find_plugin_directory, as load_plugin does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(plugins): drop the always-null last_display from plugin state info PluginStateManager reported `last_display` from `_last_display`, which nothing ever wrote, so it was null for every plugin. Recording it in PluginExecutor.execute_display would not help: get_state_info's only reader is the web process, whose PluginManager never calls display(). Remove the field, its dict and get_last_display() (no caller in core, the web UI or the plugin monorepo). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(store): share the rollback and requirements helpers, drop dead code - install_plugin and _reinstall_with_rollback set aside, discard and restore the old copy through _set_aside/_discard_backup/_restore_backup instead of two copies of the same blocks. - The loader and the store run the same pre-pip checks through contained_plugin_dir() and requirements_to_install() in plugin_loader. They still invoke pip differently (sys.executable -m pip vs. the sudo wrapper). `except (BrokenPipeError, OSError)` + `isinstance(e, OSError)` becomes `except OSError` checking errno.EPIPE. - load_module never returns None, so load_plugin's check is gone and the docstring says what it raises. - Remove the always-true JSONSCHEMA_AVAILABLE, the inline re-imports of re and permission_utils, the fake status_result object nobody reads, hasattr(git_error, 'cmd'), a redundant "merge conflict" test and `import traceback` (exc_info=True does it). - Correct comments: install_from_url names the directory for the caller's id when given (not always the manifest id), _get_local_git_info saves one git subprocess (not four), _enrich calls two helpers, search_plugins documents all its arguments, _find_plugin_path states its behaviour instead of a TODO, and history narration is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(plugins): tidy base_plugin, correct plugin_manager/state comments - base_plugin: drop the unused `import logging`; get_display_duration runs the instance value and the config value through one _positive_seconds() helper instead of two copies of the coercion; the 'static'/'none'/fallback branches of get_vegas_display_mode, which all returned FIXED_SEGMENT, are one; fix the mis-indented validate_config example; say that get_supported_vegas_modes/get_vegas_segment_width are not consulted by core (kept, plugins override them). - schema_manager: import expand_style_elements normally rather than swallowing an ImportError of a core module. - plugin_manager: the plugins directory is the configured one (plugin-repos/ by default), not plugins/; get_config() returns the live dict, not a copy, so the interval cache comments say what it saves. - state_manager: config_version and the file version are not used to detect corruption; say what they are. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(plugins): stop writing data/plugin_operations.json PluginOperationQueue wrote its finished-operation history to data/plugin_operations.json after every operation, and read it back only into its own in-memory list, which only get_operation_history() exposes -- and nothing calls that. The operation-history endpoint reads OperationHistory (data/operation_history.json). No code in src/, web_interface/, scripts/ or test/ reads the file. Drop the history_file/lazy_load parameters and the load/save code; the bounded in-memory history stays. web_interface/app.py and the integration test stop passing the removed arguments. An existing data/plugin_operations.json is left in place (data/* is gitignored). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(changelog): plugin-system Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
133 lines
5.1 KiB
Python
133 lines
5.1 KiB
Python
"""A malformed metrics cache entry must not take every plugin down with it.
|
|
|
|
`ResourceMetrics(**cached)` raises TypeError on a single unexpected key, and
|
|
that exception escapes into plugin_manager, which reports it per plugin as
|
|
"plugin <id> operation failed". Every plugin fails and the plugin system never
|
|
finishes initialising -- the health endpoint reports
|
|
`plugin_system: not_initialized` while the display itself keeps running.
|
|
|
|
Seen on a live rig, once per plugin, continuously:
|
|
|
|
ERROR - src.plugin_system.plugin_manager - plugin geochron operation failed:
|
|
ResourceMetrics.__init__() got an unexpected keyword argument
|
|
'consecutive_failures'
|
|
|
|
`consecutive_failures` belongs to plugin_health, not to metrics. How a
|
|
health-shaped record came to sit under a plugin_metrics key on that machine is
|
|
not established -- a restored backup that mixed two machines' caches is the
|
|
likeliest explanation, and the same rig had one restored onto it -- but a
|
|
loader that turns one bad cache entry into a total outage is the part worth
|
|
fixing. plugin_health already repairs its own records field by field rather
|
|
than trusting what is on disk.
|
|
"""
|
|
import logging
|
|
from dataclasses import fields
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
from src.plugin_system.resource_monitor import PluginResourceMonitor, ResourceMetrics
|
|
|
|
|
|
class _Cache:
|
|
def __init__(self, payload=None):
|
|
self.payload = payload
|
|
|
|
def get(self, key, max_age=None, memory_ttl=None, **kwargs):
|
|
return self.payload
|
|
|
|
def set(self, key, data, ttl=None, **kwargs):
|
|
pass
|
|
|
|
|
|
def _monitor(payload):
|
|
m = PluginResourceMonitor(cache_manager=_Cache(payload))
|
|
m.logger = logging.getLogger("test")
|
|
return m
|
|
|
|
|
|
#: What the rig actually had under the metrics key.
|
|
HEALTH_SHAPED = {
|
|
"consecutive_failures": 0, "circuit_state": "closed",
|
|
"circuit_opened_time": None, "half_open_start_time": None,
|
|
"last_error": None, "last_failure_time": None,
|
|
"last_success_time": 1_700_000_000.0, "total_failures": 0,
|
|
"total_successes": 42,
|
|
}
|
|
|
|
|
|
def test_a_health_record_under_the_metrics_key_does_not_raise():
|
|
"""The exact failure: it must degrade, not take the plugin system down."""
|
|
monitor = _monitor(HEALTH_SHAPED)
|
|
metrics = monitor.get_metrics(" plugin-a".strip())
|
|
assert isinstance(metrics, ResourceMetrics)
|
|
|
|
|
|
def test_recognised_fields_in_a_mixed_record_are_kept():
|
|
"""Dropping the record wholesale would lose real history unnecessarily."""
|
|
mixed = dict(HEALTH_SHAPED, call_count=7, memory_mb=12.5)
|
|
metrics = _monitor(mixed).get_metrics("plugin-b")
|
|
assert metrics.call_count == 7
|
|
assert metrics.memory_mb == 12.5
|
|
|
|
|
|
def test_a_clean_record_still_loads_unchanged():
|
|
clean = {f.name: 3 for f in fields(ResourceMetrics)}
|
|
metrics = _monitor(clean).get_metrics("plugin-c")
|
|
for name in (f.name for f in fields(ResourceMetrics)):
|
|
assert getattr(metrics, name) == 3
|
|
|
|
|
|
def test_unknown_fields_are_named_in_the_log(caplog):
|
|
"""Silently discarding them would hide a real schema change."""
|
|
with caplog.at_level(logging.WARNING):
|
|
_monitor(HEALTH_SHAPED).get_metrics("plugin-d")
|
|
# getMessage(), not .message: the latter is only populated once a handler
|
|
# formats the record, so the obvious spelling silently never matches.
|
|
assert any("consecutive_failures" in r.getMessage() for r in caplog.records), \
|
|
caplog.text
|
|
|
|
|
|
@pytest.mark.parametrize("payload", ["a string", 42, ["a", "list"]])
|
|
def test_a_non_mapping_cache_entry_does_not_raise(payload):
|
|
metrics = _monitor(payload).get_metrics("plugin-e")
|
|
assert isinstance(metrics, ResourceMetrics)
|
|
|
|
|
|
@pytest.mark.parametrize("bad", [
|
|
{"call_count": "not a number"},
|
|
{"memory_mb": None},
|
|
{"execution_time": {"nested": "junk"}},
|
|
{"min_execution_time": ["a", "list"]},
|
|
])
|
|
def test_values_of_the_wrong_type_fall_back_to_usable_defaults(bad):
|
|
"""isinstance() alone was not enough.
|
|
|
|
A dataclass does not enforce its annotations, so the bad value was simply
|
|
stored and the old assertion passed -- then monitor_call() raised
|
|
"can only concatenate str (not \"int\") to str" on the next call. The
|
|
metrics must come back *usable*, not merely constructed.
|
|
"""
|
|
monitor = _monitor(bad)
|
|
metrics = monitor.get_metrics("plugin-f")
|
|
assert isinstance(metrics, ResourceMetrics)
|
|
|
|
field_name = next(iter(bad))
|
|
assert isinstance(getattr(metrics, field_name), (int, float)), \
|
|
f"{field_name} came back as {getattr(metrics, field_name)!r}"
|
|
|
|
# The real proof: the arithmetic monitor_call and get_metrics_summary do
|
|
# on the loaded metrics must not explode.
|
|
metrics.call_count += 1
|
|
metrics.total_execution_time += 0.5
|
|
metrics.max_execution_time = max(metrics.max_execution_time, 0.5)
|
|
metrics.min_execution_time = min(metrics.min_execution_time, 0.5)
|
|
metrics.memory_mb = max(metrics.memory_mb, 1.0)
|
|
assert metrics.total_execution_time / metrics.call_count >= 0
|
|
|
|
|
|
def test_a_numeric_string_is_accepted_rather_than_discarded():
|
|
"""JSON round-trips can widen an int to a string; that is recoverable."""
|
|
metrics = _monitor({"call_count": "7"}).get_metrics("plugin-g")
|
|
assert metrics.call_count == 7
|