From c00bf5e8e6dff1e3de0b5df557839f3e67ea3d89 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Mon, 28 Sep 2026 10:41:40 -0400 Subject: [PATCH] fix(plugin-system): unload/update race, failed-load cleanup, limits validation, schema lookup, install rollback (#653) * fix(plugin-system): unload/update race, failed-load module cleanup, limits validation, schema lookup, install rollback, op-queue dedupe - unload_plugin takes the per-plugin lock (5s bounded) before cleanup(), and an update() that finishes after its plugin was unloaded no longer sets the state back to ENABLED. - A load that fails after import drops plugin_ and its submodules and forgets its manager fonts, so a fixed plugin reloads new code. - Resource limits are validated as non-negative numbers: 400 at POST /plugins/limits, bad cached records ignored with one warning. Route docstrings note health/metrics reset and limits only change the web process's view. - SchemaManager.get_schema_path resolves each search dir via resolve_plugin_dir (manifest id, ledmatrix-) before the literal paths; plugins/ still before plugin-repos/. Misses cached 30s and logged once at DEBUG. - install_from_url sets an existing copy aside and restores it if the move fails, under the per-plugin reinstall lock. - Operation queue refuses a second pending op for a plugin and trims _operations with history. - get_vegas_render_width reads display_manager.width first. - get_logger in store/schema/health/resource/saved_repositories; UTF-8 reads in store_manager and state_manager. - Docs: update_interval precedence (manifest over config) stated where users are told to set it in config. Co-Authored-By: Claude Opus 5.5 * fix(web): build the limits 400 message from the field name, not an exception CodeQL flagged str(e) flowing into the response. invalid_limit_field() returns the offending field without raising, and limits_from_dict uses it. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 10 +++ docs/PLUGIN_CONFIGURATION_GUIDE.md | 8 ++ docs/REST_API_REFERENCE.md | 8 +- docs/TROUBLESHOOTING.md | 9 ++ src/plugin_system/base_plugin.py | 21 +++-- src/plugin_system/compatibility.py | 2 + src/plugin_system/operation_queue.py | 18 ++++ src/plugin_system/plugin_health.py | 5 +- src/plugin_system/plugin_manager.py | 67 +++++++++++++++ src/plugin_system/resource_monitor.py | 65 +++++++++++++- src/plugin_system/saved_repositories.py | 4 +- src/plugin_system/schema_manager.py | 70 +++++++++++++-- src/plugin_system/state_manager.py | 2 +- src/plugin_system/store_manager.py | 47 ++++++---- test/test_async_plugin_updates.py | 45 ++++++++++ test/test_base_plugin_vegas_render_width.py | 48 +++++++++++ test/test_compatibility.py | 5 ++ test/test_operation_queue_pending_and_trim.py | 71 +++++++++++++++ ...test_plugin_manager_failed_load_cleanup.py | 75 ++++++++++++++++ test/test_plugin_system_utf8_reads.py | 62 +++++++++++++ test/test_resource_limits_validation.py | 81 +++++++++++++++++ test/test_schema_path_resolution.py | 68 +++++++++++++++ test/test_store_install_from_url_rollback.py | 86 +++++++++++++++++++ web_interface/blueprints/api_v3/plugins.py | 41 ++++++--- 24 files changed, 869 insertions(+), 49 deletions(-) create mode 100644 test/test_base_plugin_vegas_render_width.py create mode 100644 test/test_operation_queue_pending_and_trim.py create mode 100644 test/test_plugin_manager_failed_load_cleanup.py create mode 100644 test/test_plugin_system_utf8_reads.py create mode 100644 test/test_resource_limits_validation.py create mode 100644 test/test_schema_path_resolution.py create mode 100644 test/test_store_install_from_url_rollback.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5057fd5d..66125ae3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,16 @@ accepts both, but the store flags the old spelling as deprecated ## Unreleased +- Plugin system fixes: + - Unloading a plugin waits (up to 5s) for an in-flight `update()` before running `cleanup()`/`on_disable()`, and an update that finishes after the unload no longer puts the plugin back to ENABLED. + - A plugin whose load fails after its module was imported (constructor, `validate_config()` or `on_enable()` raising) no longer leaves that module cached: fixing the plugin and reloading it runs the new code without a restart. Its font registrations are dropped too. + - `POST /api/v3/plugins/limits/` answers 400 for a limit that isn't a non-negative number (a string limit used to make every later update of that plugin raise). A bad cached limits record is ignored with a warning instead of raising. + - The config schema is found for a plugin installed as `ledmatrix-` or in a directory named differently from its manifest id, resolved the way the loader resolves it (plugins/ is still searched before plugin-repos/). A plugin with no schema is logged once at DEBUG instead of a warning on every lookup. + - Installing from a URL over an existing install sets the old copy aside and restores it if the move fails, under the same per-plugin lock as a registry install. + - The operation queue refuses a second operation for a plugin whose first is still waiting (a double-clicked Install ran twice), and no longer keeps every finished operation in memory. + - `get_vegas_render_width()` reads `display_manager.width` first, as plugins are told to. + - Store and state files are read as UTF-8 regardless of the system locale. + - Docs: `update_interval` in `config.json` sets the scheduler's cadence only for a plugin whose manifest has none (TROUBLESHOOTING, PLUGIN_CONFIGURATION_GUIDE). The health/metrics reset and limits routes note that they only change the web process's view. - Web UI cleanup and dependency pins: - A plugin's own config widget (`/static/plugin-widgets//.js`) is requested with `?v=`, so an updated plugin's widget reaches browsers instead of the copy cached as immutable for a year. - A failed installed-plugins reload after a toggle, install or uninstall shows one error, not a second generic "unexpected error" toast. diff --git a/docs/PLUGIN_CONFIGURATION_GUIDE.md b/docs/PLUGIN_CONFIGURATION_GUIDE.md index 1a28c04f..f35c0e67 100644 --- a/docs/PLUGIN_CONFIGURATION_GUIDE.md +++ b/docs/PLUGIN_CONFIGURATION_GUIDE.md @@ -124,6 +124,14 @@ Plugins are configured by adding their plugin ID as a top-level key in the confi } ``` +How often the core calls a plugin's `update()`: the plugin's +`get_update_interval()` if it returns a number, else `update_interval` in the +plugin's `manifest.json`, else `update_interval` in its `config.json` section +as above, else 60 seconds. A config `update_interval` therefore only sets the +scheduler's cadence for a plugin whose manifest does not; plugins that expose +it in their config schema typically also honour it themselves inside +`update()`. See [PLUGIN_API_REFERENCE.md](PLUGIN_API_REFERENCE.md#get_update_interval---optionalfloat). + ### Plugin Display Durations Add plugin display modes to the `display_durations` section: diff --git a/docs/REST_API_REFERENCE.md b/docs/REST_API_REFERENCE.md index f6a72aa6..fbfd505f 100644 --- a/docs/REST_API_REFERENCE.md +++ b/docs/REST_API_REFERENCE.md @@ -869,7 +869,13 @@ Get a plugin's resource limits. `data` is `null` when none are configured. **POST** `/api/v3/plugins/limits/` Set a plugin's resource limits. The body replaces all four limits: a key you -omit is stored as no limit (`warning_threshold` defaults to `0.8`). +omit is stored as no limit (`warning_threshold` defaults to `0.8`). Each value +must be a non-negative number or `null`; anything else is a 400. + +The limits are stored in the shared cache. A display service that has already +read limits for the plugin keeps using those until it restarts; likewise the +health and metrics reset routes clear the stored record and the web process's +copy, not the display service's in-memory state. **Request Body**: ```json diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index fede8dbb..1d126eed 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -616,6 +616,15 @@ sudo systemctl cat ledmatrix-web | grep User ``` **Note:** Minimum recommended: 300 seconds (5 minutes) + How often the core calls the plugin's `update()` comes from the plugin + itself first: its `get_update_interval()` if it has one, then + `update_interval` in its `manifest.json`. The `update_interval` in + `config.json` is used by the scheduler only when the manifest sets none. + Many plugins also read their own config `update_interval` and skip the + API call inside `update()` until it has elapsed, which is what makes the + setting above effective; check the plugin's settings form or + `config_schema.json` for the option it actually honours. + 2. **Check current rate limit usage:** - OpenWeatherMap free tier: 1,000 calls/day, 60 calls/minute - With 300s interval: 288 calls/day (well within limits) diff --git a/src/plugin_system/base_plugin.py b/src/plugin_system/base_plugin.py index 5af7f7fe..8ab9abdf 100644 --- a/src/plugin_system/base_plugin.py +++ b/src/plugin_system/base_plugin.py @@ -776,28 +776,35 @@ class BasePlugin(ABC): tighter arrangement instead of being cropped afterwards. Vegas also narrows ``display_manager`` for the duration of the call, so - a plugin that already sizes itself from ``matrix.width`` needs no - changes. Read this only when you size content some other way. + a plugin that already sizes itself from ``display_manager.width`` needs + no changes. Read this only when you size content some other way. Controlled by the plugin's own ``vegas_width_pct`` config value, else the global ``display.vegas_scroll.render_width_pct``. Returns: Target width in pixels. Outside a Vegas content request, the full - display width. + display width: ``display_manager.width``, which falls back to the + canvas size when ``matrix`` is None (hardware init failed). """ requested = getattr(self, '_vegas_render_width', None) if isinstance(requested, int) and requested > 0: return requested + # display_manager.width first, as CLAUDE.md asks of every plugin: it + # already reads matrix.width when there is a matrix. matrix.width is + # only the fallback for a display_manager without a width (a test + # double, an older wrapper). display_manager = getattr(self, 'display_manager', None) - matrix = getattr(display_manager, 'matrix', None) - if matrix is not None and getattr(matrix, 'width', None): - return int(matrix.width) width = getattr(display_manager, 'width', None) if callable(width): width = width() - return int(width) if width else 128 + if width: + return int(width) + matrix = getattr(display_manager, 'matrix', None) + if matrix is not None and getattr(matrix, 'width', None): + return int(matrix.width) + return 128 def get_vegas_content(self) -> Optional[Any]: """ diff --git a/src/plugin_system/compatibility.py b/src/plugin_system/compatibility.py index 36677c41..28a5a4f2 100644 --- a/src/plugin_system/compatibility.py +++ b/src/plugin_system/compatibility.py @@ -97,6 +97,8 @@ def parse_semver(value: Any) -> Optional[Tuple[int, int, int]]: try: nums = [int(''.join(ch for ch in p if ch.isdigit()) or 0) for p in parts[:3]] except ValueError: + # Reachable: str.isdigit() accepts characters int() rejects, such as + # a superscript "\u00b2" -- "1.\u00b2.0" lands here. return None while len(nums) < 3: nums.append(0) diff --git a/src/plugin_system/operation_queue.py b/src/plugin_system/operation_queue.py index 38dae2d1..7bd24b78 100644 --- a/src/plugin_system/operation_queue.py +++ b/src/plugin_system/operation_queue.py @@ -85,6 +85,17 @@ class PluginOperationQueue: f"Plugin {plugin_id} already has an active operation: " f"{active_op.operation_id} ({active_op.operation_type.value})" ) + + # _active_operations only holds the *running* one, so a second + # request while the first still waits in the queue (a double- + # clicked Install) used to be queued too, and both ran back to + # back. Refuse it the same way. + for queued_op in self._operations.values(): + if queued_op.plugin_id == plugin_id and queued_op.status == OperationStatus.PENDING: + raise ValueError( + f"Plugin {plugin_id} already has an active operation: " + f"{queued_op.operation_id} ({queued_op.operation_type.value})" + ) # Create operation operation = PluginOperation( @@ -288,7 +299,14 @@ class PluginOperationQueue: if len(self._operation_history) > self.max_history: # Remove oldest operations self._operation_history.sort(key=lambda op: op.created_at) + dropped = self._operation_history[:-self.max_history] self._operation_history = self._operation_history[-self.max_history:] + # ...and forget them in the status map too, which otherwise kept + # every operation ever enqueued for the life of the process. A + # still-pending or running one is never dropped from lookups. + for op in dropped: + if op.status not in (OperationStatus.PENDING, OperationStatus.RUNNING): + self._operations.pop(op.operation_id, None) def shutdown(self) -> None: """Shutdown the operation queue and worker thread.""" diff --git a/src/plugin_system/plugin_health.py b/src/plugin_system/plugin_health.py index 3908d9ca..ec4b3c0f 100644 --- a/src/plugin_system/plugin_health.py +++ b/src/plugin_system/plugin_health.py @@ -6,10 +6,11 @@ and circuit breaker state. Provides automatic recovery mechanisms. """ import time -import logging from typing import Dict, Optional, Any, Tuple from enum import Enum +from src.logging_config import get_logger + class CircuitState(Enum): """Circuit breaker states.""" @@ -43,7 +44,7 @@ class PluginHealthTracker: self.failure_threshold = failure_threshold self.cooldown_period = cooldown_period self.half_open_timeout = half_open_timeout - self.logger = logging.getLogger(__name__) + self.logger = get_logger(__name__) # In-memory health state (also persisted to cache) self._health_state: Dict[str, Dict[str, Any]] = {} diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index d06c347e..6bd798d8 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -52,6 +52,10 @@ class PluginManager: - PluginExecutor: Handles plugin execution with timeout and error isolation - PluginStateManager: Manages plugin state machine """ + + # How long unload_plugin() waits for an in-flight update() to finish + # before tearing the instance down anyway. + UNLOAD_LOCK_TIMEOUT = 5.0 def __init__(self, plugins_dir: str = "plugins", config_manager: Optional[Any] = None, @@ -408,10 +412,12 @@ class PluginManager: try: if not plugin_instance.validate_config(): self.logger.error("Plugin %s configuration validation failed", plugin_id) + self._discard_failed_load(plugin_id) self.state_manager.set_state(plugin_id, PluginState.ERROR) return False except Exception as e: self.logger.error("Error validating plugin %s config: %s", plugin_id, e, exc_info=True) + self._discard_failed_load(plugin_id) self.state_manager.set_state(plugin_id, PluginState.ERROR, error=e) return False @@ -456,12 +462,34 @@ class PluginManager: except PluginError as e: self.logger.error("Plugin error loading %s: %s", plugin_id, e, exc_info=True) + self._discard_failed_load(plugin_id) self.state_manager.set_state(plugin_id, PluginState.ERROR, error=e) return False except Exception as e: self.logger.error("Unexpected error loading plugin %s: %s", plugin_id, e, exc_info=True) + self._discard_failed_load(plugin_id) self.state_manager.set_state(plugin_id, PluginState.ERROR, error=e) return False + + def _discard_failed_load(self, plugin_id: str) -> None: + """Forget a plugin's imported module and font registrations after a + failed load. + + load_module() reuses ``plugin_`` from sys.modules, so a module + left behind by a load that failed after import (instantiation, + validate_config, on_enable) would keep serving the old code even + after the user fixes the plugin and reloads it. Never raises. + """ + try: + sys.modules.pop(f"plugin_{plugin_id.replace('-', '_')}", None) + self.plugin_loader.unregister_plugin_modules(plugin_id) + except Exception as e: # pragma: no cover - defensive + self.logger.debug("Could not drop modules of %s: %s", plugin_id, e) + try: + if self.font_manager is not None and hasattr(self.font_manager, 'forget_manager_fonts'): + self.font_manager.forget_manager_fonts(plugin_id) + except Exception as e: + self.logger.debug("Could not forget fonts of %s: %s", plugin_id, e) #: Config keys the **core** reads out of a plugin's own config block. The #: plugin never declares them, so a schema with @@ -607,6 +635,28 @@ class PluginManager: self.logger.warning("Plugin %s not loaded", plugin_id) return False + # Take the plugin's lock so cleanup()/on_disable() can't run while + # the update worker is mid-update() on this instance. Bounded: an + # update() that hangs past PluginExecutor's timeout keeps holding the + # lock from its lingering thread, and unload must still go through. + lock = self.get_plugin_lock(plugin_id) + lock_acquired = lock.acquire(timeout=self.UNLOAD_LOCK_TIMEOUT) + if not lock_acquired: + self.logger.warning( + "Plugin %s still busy after %.1fs; unloading without its lock", + plugin_id, self.UNLOAD_LOCK_TIMEOUT) + try: + return self._unload_plugin_locked(plugin_id) + finally: + if lock_acquired: + lock.release() + + def _unload_plugin_locked(self, plugin_id: str) -> bool: + """Body of unload_plugin(); caller holds (or gave up on) the plugin lock.""" + if plugin_id not in self.plugins: # unloaded while we waited + self.logger.warning("Plugin %s not loaded", plugin_id) + return False + try: plugin = self.plugins[plugin_id] @@ -905,6 +955,14 @@ class PluginManager: updating, since a scheduler that propagates a plugin bug stops every other plugin too. + Precedence, first match wins: the ``get_update_interval()`` hook, then + ``update_interval`` in the plugin's **manifest**, then + ``update_interval`` in the plugin's section of config.json, then 60s. + So a config value only drives the scheduler for a plugin whose + manifest sets none; when the manifest sets one, the config value is + ignored here (a plugin may still read it itself, e.g. to skip fetches + inside update()). + The static result is cached per plugin_id after the first lookup, so the manifest/config resolution is not repeated on every scheduling tick of the display loop. A change to ``update_interval`` in @@ -1206,6 +1264,15 @@ class PluginManager: return finished['done'] = True try: + # The plugin was unloaded (or reloaded as a new instance) + # while this update() ran: unload_plugin() already cleared its + # lifecycle state, so recording success/failure here would + # resurrect a torn-down plugin as ENABLED. Only release. + if self.plugins.get(plugin_id) is not plugin_instance: + if lock is not None: + with self._pending_lock: + self._pending_updates.discard(plugin_id) + return # Drop the queue reservation *before* the state goes back to # ENABLED. The other order leaves a window where a scheduler # sees ENABLED, reserves the plugin, then finds it still in diff --git a/src/plugin_system/resource_monitor.py b/src/plugin_system/resource_monitor.py index ed11e9f3..adeb6ed6 100644 --- a/src/plugin_system/resource_monitor.py +++ b/src/plugin_system/resource_monitor.py @@ -5,12 +5,14 @@ Tracks resource usage (memory, CPU, execution time) for plugins. Provides resource limits and performance monitoring. """ +import math import time -import logging import threading from typing import Dict, Optional, Any, Callable from dataclasses import dataclass, field, fields +from src.logging_config import get_logger + try: import psutil PSUTIL_AVAILABLE = True @@ -31,6 +33,52 @@ class ResourceLimits: warning_threshold: float = 0.8 # Warning at 80% of limit +_LIMIT_FIELDS = ('max_memory_mb', 'max_cpu_percent', 'max_execution_time', + 'warning_threshold') + + +def invalid_limit_field(data: Any) -> Optional[str]: + """The first field of a limits mapping that isn't a valid limit, or None. + + ``"limits"`` when ``data`` isn't a mapping at all. Separate from + limits_from_dict so a caller can report the problem without passing an + exception's text back to a client. + """ + if not isinstance(data, dict): + return 'limits' + for name in _LIMIT_FIELDS: + value = data.get(name) + if value is None: + continue + # bool is an int subclass; True is not a limit anyone meant. + if (isinstance(value, bool) or not isinstance(value, (int, float)) + or not math.isfinite(value) or value < 0): + return name + return None + + +def limits_from_dict(data: Any) -> ResourceLimits: + """Build ResourceLimits from a JSON-shaped mapping, validating each value. + + A dataclass does not enforce its annotations, so ResourceLimits built from + raw request JSON or a cached record happily stores ``"50"`` -- and then + every monitored update() raises TypeError comparing a float with it. Each + ``max_*`` value must be absent/None (no limit) or a non-negative number; + ``warning_threshold`` defaults to 0.8. Unknown keys are ignored. + + Raises: + ValueError: naming the first offending field. + """ + bad = invalid_limit_field(data) + if bad == 'limits': + raise ValueError(f"limits must be an object, got {type(data).__name__}") + if bad: + raise ValueError( + f"{bad} must be a non-negative number or null, got {data.get(bad)!r}") + return ResourceLimits(**{name: data[name] for name in _LIMIT_FIELDS + if data.get(name) is not None}) + + @dataclass class ResourceMetrics: """Resource usage metrics for a plugin. @@ -86,11 +134,12 @@ class PluginResourceMonitor: """ self.cache_manager = cache_manager self.enable_monitoring = enable_monitoring and PSUTIL_AVAILABLE - self.logger = logging.getLogger(__name__) + self.logger = get_logger(__name__) # Resource metrics per plugin self._metrics: Dict[str, ResourceMetrics] = {} self._limits: Dict[str, ResourceLimits] = {} + self._bad_limits_warned: set = set() # When each plugin's metrics last reached the cache. Metrics change on # every call, so they cannot be de-duplicated the way health state can; # they are rate-limited instead. See _METRICS_PERSIST_INTERVAL. @@ -230,7 +279,17 @@ class PluginResourceMonitor: cache_key = self._get_limits_key(plugin_id) cached = self.cache_manager.get(cache_key, max_age=None) if cached: - self._limits[plugin_id] = ResourceLimits(**cached) + try: + self._limits[plugin_id] = limits_from_dict(cached) + except ValueError as e: + # Treat as no limits rather than letting every update + # of this plugin raise; warn once, not on every call. + if plugin_id not in self._bad_limits_warned: + self._bad_limits_warned.add(plugin_id) + self.logger.warning( + "Ignoring cached resource limits for %s: %s", + plugin_id, e) + return None else: return None return self._limits[plugin_id] diff --git a/src/plugin_system/saved_repositories.py b/src/plugin_system/saved_repositories.py index 64794d8c..86dcd250 100644 --- a/src/plugin_system/saved_repositories.py +++ b/src/plugin_system/saved_repositories.py @@ -5,11 +5,11 @@ Manages saved GitHub repository URLs for easy plugin discovery and installation. """ import json -import logging import os from pathlib import Path from typing import List, Dict, Optional +from src.logging_config import get_logger from src.plugin_system.repo_urls import normalize_repo_url @@ -24,7 +24,7 @@ class SavedRepositoriesManager: config_path: Path to JSON file storing saved repositories """ self.config_path = Path(config_path) - self.logger = logging.getLogger(__name__) + self.logger = get_logger(__name__) self.repositories = self._load_repositories() def _load_repositories(self) -> List[Dict[str, str]]: diff --git a/src/plugin_system/schema_manager.py b/src/plugin_system/schema_manager.py index 8b267e7c..b146d019 100644 --- a/src/plugin_system/schema_manager.py +++ b/src/plugin_system/schema_manager.py @@ -8,6 +8,7 @@ Provides utilities for extracting defaults, validating configurations, and manag import copy import json import logging +import time from pathlib import Path from typing import Any, Dict, List, Optional, Tuple import jsonschema @@ -15,6 +16,8 @@ from jsonschema import Draft7Validator, ValidationError from src.core_config_keys import CORE_CONFIG_KEYS from src.element_style import expand_style_elements +from src.logging_config import get_logger +from src.plugin_system.plugin_dirs import resolve_plugin_dir def _renders_as_object(prop: Dict[str, Any]) -> bool: @@ -367,7 +370,7 @@ class SchemaManager: device-wide ``location`` that seeds plugin location defaults. Omitting it simply leaves schema defaults untouched. """ - self.logger = logger or logging.getLogger(__name__) + self.logger = logger or get_logger(__name__) self.plugins_dir = plugins_dir self.project_root = project_root or Path.cwd() self.config_manager = config_manager @@ -377,23 +380,67 @@ class SchemaManager: # Default config cache: plugin_id -> default config dict self._defaults_cache: Dict[str, Dict[str, Any]] = {} - + + # Schema-path misses: plugin_id -> monotonic time of the miss. A + # lookup now scans each search directory's manifests, and plugins + # without a schema are asked about on every page render. + self._schema_path_misses: Dict[str, float] = {} + self._schema_miss_logged: set = set() + + #: How long a "no schema" answer is reused before the directories are + #: searched again -- short, so a plugin installed by a path that doesn't + #: call invalidate_cache() (a dev symlink, a manual copy) still shows up. + SCHEMA_MISS_TTL = 30.0 + def get_schema_path(self, plugin_id: str) -> Optional[Path]: """ Get the path to a plugin's config_schema.json file. - - Tries multiple locations in order: + + Each search directory -- plugins_dir, then PROJECT_ROOT/plugins, then + PROJECT_ROOT/plugin-repos -- is first resolved the way the plugin + loader resolves it (``plugin_dirs.resolve_plugin_dir``: the directory + whose manifest declares the id, else ```` / ``ledmatrix-``, + case-insensitively). Only if none of those holds a schema are the + literal locations tried: 1. plugins_dir / plugin_id / config_schema.json 2. PROJECT_ROOT / plugins / plugin_id / config_schema.json 3. PROJECT_ROOT / plugin-repos / plugin_id / config_schema.json - + 4. a case-insensitive match of plugin_id in plugins/ and plugin-repos/ + + A miss is remembered for SCHEMA_MISS_TTL seconds (or until + invalidate_cache()) and logged once, at DEBUG: PluginManager already + warns at load time about a plugin that ships no schema. + Args: plugin_id: Plugin identifier - + Returns: Path to schema file or None if not found """ + missed_at = self._schema_path_misses.get(plugin_id) + if missed_at is not None and time.monotonic() - missed_at < self.SCHEMA_MISS_TTL: + return None + + search_dirs = [] + if self.plugins_dir: + search_dirs.append(Path(self.plugins_dir)) + search_dirs.extend([self.project_root / 'plugins', + self.project_root / 'plugin-repos']) + + # Resolved the way the loader does, so a plugin installed as + # ``ledmatrix-`` or under a directory named differently from its + # manifest id still gets its schema. One directory at a time keeps + # the documented plugins/-before-plugin-repos/ order. possible_paths = [] + for search_dir in search_dirs: + try: + resolved = resolve_plugin_dir( + plugin_id, [search_dir], prefix=True, case_insensitive=True) + except Exception as e: # pragma: no cover - defensive + self.logger.debug(f"Could not resolve {plugin_id} in {search_dir}: {e}") + resolved = None + if resolved is not None: + possible_paths.append(resolved / 'config_schema.json') # Try plugins_dir if set if self.plugins_dir: @@ -416,9 +463,14 @@ class SchemaManager: for path in possible_paths: if path.exists(): self.logger.debug(f"Found schema for {plugin_id} at {path}") + self._schema_path_misses.pop(plugin_id, None) + self._schema_miss_logged.discard(plugin_id) return path - - self.logger.warning(f"Schema file not found for plugin {plugin_id}") + + self._schema_path_misses[plugin_id] = time.monotonic() + if plugin_id not in self._schema_miss_logged: + self._schema_miss_logged.add(plugin_id) + self.logger.debug(f"Schema file not found for plugin {plugin_id}") return None def load_schema(self, plugin_id: str, use_cache: bool = True) -> Optional[Dict[str, Any]]: @@ -481,10 +533,12 @@ class SchemaManager: if plugin_id: self._schema_cache.pop(plugin_id, None) self._defaults_cache.pop(plugin_id, None) + self._schema_path_misses.pop(plugin_id, None) self.logger.debug(f"Invalidated cache for plugin {plugin_id}") else: self._schema_cache.clear() self._defaults_cache.clear() + self._schema_path_misses.clear() self.logger.debug("Invalidated all schema caches") def extract_defaults_from_schema(self, schema: Dict[str, Any], prefix: str = '') -> Dict[str, Any]: diff --git a/src/plugin_system/state_manager.py b/src/plugin_system/state_manager.py index 35d9feee..8e8243a8 100644 --- a/src/plugin_system/state_manager.py +++ b/src/plugin_system/state_manager.py @@ -320,7 +320,7 @@ class PluginStateManager: return try: - with open(self.state_file, 'r') as f: + with open(self.state_file, 'r', encoding='utf-8') as f: state_data = json.load(f) with self._lock: diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 204aca57..d6da4948 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -21,10 +21,10 @@ from concurrent.futures import ThreadPoolExecutor from datetime import datetime from pathlib import Path from typing import List, Dict, Optional, Any, Tuple, Set -import logging from jsonschema import Draft7Validator, ValidationError +from src.logging_config import get_logger from src.common.permission_utils import ( ensure_directory_permissions, get_plugin_dir_mode, install_requirements_file, sudo_remove_directory, @@ -79,7 +79,7 @@ class PluginStoreManager: ``config/uninstalled_plugins.json`` under the project root. """ self.plugins_dir = Path(plugins_dir) - self.logger = logging.getLogger(__name__) + self.logger = get_logger(__name__) self.registry_cache = None self.registry_cache_time = None # Timestamp of when registry was cached self.github_cache = {} # Cache for GitHub API responses @@ -333,7 +333,7 @@ class PluginStoreManager: try: config_path = Path(__file__).parent.parent.parent / "config" / "config_secrets.json" if config_path.exists(): - with open(config_path, 'r') as f: + with open(config_path, 'r', encoding='utf-8') as f: config = json.load(f) token = config.get('github', {}).get('api_token', '').strip() if token and token != "YOUR_GITHUB_PERSONAL_ACCESS_TOKEN": @@ -1528,7 +1528,7 @@ class PluginStoreManager: 'error': 'No manifest.json found in repository' + (f' at path: {plugin_path}' if plugin_path else '') } - with open(manifest_path, 'r') as f: + with open(manifest_path, 'r', encoding='utf-8') as f: manifest = json.load(f) requested_id = plugin_id @@ -1590,7 +1590,7 @@ class PluginStoreManager: if 'entry_point' not in manifest: manifest['entry_point'] = 'manager.py' # Write updated manifest back to file - with open(manifest_path, 'w') as f: + with open(manifest_path, 'w', encoding='utf-8') as f: json.dump(manifest, f, indent=2) self.logger.info(f"Added missing entry_point field to {plugin_id} manifest (defaulted to manager.py)") @@ -1599,16 +1599,33 @@ class PluginStoreManager: # manifest's id -- so it can differ from the manifest id, which # discovery tolerates by reading the manifest. final_path = self.plugins_dir / plugin_id - if final_path.exists(): - self.logger.warning(f"Plugin {plugin_id} already exists, removing existing copy") - if not self._safe_remove_directory(final_path): - return { - 'success': False, - 'error': f'Failed to remove existing plugin directory: {final_path}' - } - - shutil.move(str(temp_dir), str(final_path)) - temp_dir = None # Prevent cleanup since we moved it + # Set the existing copy aside rather than deleting it, and put it + # back if the move fails: deleting first left the user with no + # plugin at all whenever the move broke part-way. Under the + # per-plugin reinstall lock, as install_plugin() is, so two + # overlapping installs of one id can't interleave their renames. + with self._get_reinstall_lock(plugin_id): + backup_path = None + if final_path.exists(): + self.logger.warning(f"Plugin {plugin_id} already exists, replacing existing copy") + backup_path = final_path.with_name( + f"{final_path.name}{BACKUP_MARKER}preinstall") + problem = self._set_aside(final_path, backup_path) + if problem: + return { + 'success': False, + 'error': f'Failed to replace existing plugin directory: {problem}' + } + + try: + shutil.move(str(temp_dir), str(final_path)) + except Exception: + if backup_path is not None: + self._restore_backup(plugin_id, final_path, backup_path, "Install") + raise + temp_dir = None # Prevent cleanup since we moved it + if backup_path is not None: + self._discard_backup(plugin_id, backup_path, "install") # Install dependencies self._install_dependencies(final_path) diff --git a/test/test_async_plugin_updates.py b/test/test_async_plugin_updates.py index a27fea11..b908749f 100644 --- a/test/test_async_plugin_updates.py +++ b/test/test_async_plugin_updates.py @@ -254,6 +254,51 @@ class TestFailurePaths: assert target_id not in pm._pending_updates assert pm.state_manager.get_state(target_id) == PluginState.UNLOADED + def test_unload_waits_for_in_flight_update(self, pm): + """unload_plugin() must not run cleanup() while update() is still + executing on the same instance -- it waits on the plugin lock.""" + observed = {} + + class CleanupPlugin(SlowPlugin): + def cleanup(self): + observed['in_update_at_cleanup'] = self.in_update + + plugin = CleanupPlugin(update_seconds=0.5) + plugin_id = _install(pm, plugin) + pm._enqueue_update(plugin_id, time.time()) + deadline = time.monotonic() + 2 + while not plugin.in_update and time.monotonic() < deadline: + time.sleep(0.01) + assert plugin.in_update + + assert pm.unload_plugin(plugin_id) is True + assert observed == {'in_update_at_cleanup': False} + + def test_update_finishing_after_unload_does_not_resurrect(self, pm): + """When unload gives up waiting for a hung update(), that update's + eventual completion must not flip the cleared state back to ENABLED.""" + pm.UNLOAD_LOCK_TIMEOUT = 0.05 + plugin = SlowPlugin(update_seconds=0.5) + plugin_id = _install(pm, plugin) + pm._enqueue_update(plugin_id, time.time()) + deadline = time.monotonic() + 2 + while not plugin.in_update and time.monotonic() < deadline: + time.sleep(0.01) + assert plugin.in_update + + assert pm.unload_plugin(plugin_id) is True + deadline = time.monotonic() + 3 + while plugin.update_calls and plugin.in_update and time.monotonic() < deadline: + time.sleep(0.02) + time.sleep(0.2) # let _finish() run + + assert pm.state_manager.get_state(plugin_id) == PluginState.UNLOADED + assert plugin_id not in pm.plugin_last_update + assert plugin_id not in pm._pending_updates + lock = pm.get_plugin_lock(plugin_id) + assert lock.acquire(blocking=False) is True + lock.release() + class TestKillSwitch: def test_synchronous_mode_blocks_like_before(self, pm): diff --git a/test/test_base_plugin_vegas_render_width.py b/test/test_base_plugin_vegas_render_width.py new file mode 100644 index 00000000..5c547701 --- /dev/null +++ b/test/test_base_plugin_vegas_render_width.py @@ -0,0 +1,48 @@ +"""BasePlugin.get_vegas_render_width reads display_manager.width first. + +CLAUDE.md asks plugins to size themselves from ``display_manager.width`` -- +not ``display_manager.matrix.width`` -- because ``matrix`` is None when +hardware init fails and the property falls back to the canvas size. The base +class's own helper read ``matrix.width`` first. For the real DisplayManager +the two agree whenever there is a matrix, so this pins the documented order +with a display manager where they differ. +""" + +from types import SimpleNamespace + +from src.plugin_system.base_plugin import BasePlugin + + +class _Plugin(BasePlugin): + def update(self): + pass + + def display(self, force_clear=False): + pass + + +def _plugin(display_manager): + plugin = object.__new__(_Plugin) + plugin.display_manager = display_manager + return plugin + + +def test_prefers_display_manager_width_over_matrix_width(): + dm = SimpleNamespace(width=64, matrix=SimpleNamespace(width=128)) + assert _plugin(dm).get_vegas_render_width() == 64 + + +def test_uses_display_manager_width_when_matrix_is_none(): + dm = SimpleNamespace(width=96, matrix=None) + assert _plugin(dm).get_vegas_render_width() == 96 + + +def test_falls_back_to_matrix_width_without_a_width(): + dm = SimpleNamespace(matrix=SimpleNamespace(width=80)) + assert _plugin(dm).get_vegas_render_width() == 80 + + +def test_vegas_request_still_wins(): + plugin = _plugin(SimpleNamespace(width=192, matrix=None)) + plugin._vegas_render_width = 100 + assert plugin.get_vegas_render_width() == 100 diff --git a/test/test_compatibility.py b/test/test_compatibility.py index 34f88869..1425a4e0 100644 --- a/test/test_compatibility.py +++ b/test/test_compatibility.py @@ -29,6 +29,11 @@ class TestParseSemver: def test_leading_v_tolerated(self): assert parse_semver("v3.2.1") == (3, 2, 1) + def test_non_decimal_digit_is_unparseable(self): + # str.isdigit() accepts "²" but int() does not: the ValueError + # branch is reachable and must keep returning None, not raise. + assert parse_semver("1.².0") is None + def test_prerelease_suffix_stripped(self): # "3.2.0-rc1" must NOT parse as (3, 2, 1) — a release candidate must # not rank above its own release. diff --git a/test/test_operation_queue_pending_and_trim.py b/test/test_operation_queue_pending_and_trim.py new file mode 100644 index 00000000..15559732 --- /dev/null +++ b/test/test_operation_queue_pending_and_trim.py @@ -0,0 +1,71 @@ +"""PluginOperationQueue refuses a second queued op per plugin and stays bounded. + +enqueue_operation only checked _active_operations, which holds the operation +that is *running*. While a plugin's first operation still waited in the queue +(the worker busy with another plugin), a second one for the same plugin was +accepted and both ran back to back. And _operations kept every operation ever +enqueued, although the history beside it was trimmed to max_history. +""" + +import threading +import time + +import pytest + +from src.plugin_system.operation_queue import PluginOperationQueue +from src.plugin_system.operation_types import OperationStatus, OperationType + + +@pytest.fixture +def op_queue(): + q = PluginOperationQueue(max_history=3) + yield q + q.shutdown() + + +def _wait_for(predicate, timeout=5.0): + deadline = time.monotonic() + timeout + while not predicate() and time.monotonic() < deadline: + time.sleep(0.01) + return predicate() + + +def test_second_pending_operation_for_a_plugin_is_refused(op_queue): + release = threading.Event() + started = threading.Event() + + def blocker(op): + started.set() + release.wait(5) + return {"success": True} + + op_queue.enqueue_operation(OperationType.INSTALL, "busy", operation_callback=blocker) + assert started.wait(5) + + first = op_queue.enqueue_operation( + OperationType.INSTALL, "demo", operation_callback=lambda op: {"success": True}) + assert op_queue.get_operation_status(first).status == OperationStatus.PENDING + + with pytest.raises(ValueError, match="already has an active operation"): + op_queue.enqueue_operation( + OperationType.INSTALL, "demo", operation_callback=lambda op: {"success": True}) + + release.set() + assert _wait_for(lambda: op_queue.get_operation_status(first).status + == OperationStatus.COMPLETED) + # Once it has finished, the plugin accepts a new operation again. + op_queue.enqueue_operation(OperationType.UPDATE, "demo") + + +def test_operations_map_is_trimmed_with_history(op_queue): + ids = [op_queue.enqueue_operation(OperationType.INSTALL, f"p{i}", + operation_callback=lambda op: {"success": True}) + for i in range(8)] + assert _wait_for(lambda: all( + (op_queue.get_operation_status(i) is None + or op_queue.get_operation_status(i).status == OperationStatus.COMPLETED) + for i in ids) and len(op_queue.get_operation_history()) == 3) + + assert len(op_queue._operations) == 3 + kept = {op.operation_id for op in op_queue.get_operation_history()} + assert set(op_queue._operations) == kept diff --git a/test/test_plugin_manager_failed_load_cleanup.py b/test/test_plugin_manager_failed_load_cleanup.py new file mode 100644 index 00000000..a5e10824 --- /dev/null +++ b/test/test_plugin_manager_failed_load_cleanup.py @@ -0,0 +1,75 @@ +"""A load that fails after the plugin module was imported must not leave +that module behind. + +PluginLoader.load_module() reuses ``plugin_`` from sys.modules. When +instantiation or validate_config() failed, the half-loaded module stayed +there (and in the loader's ``_loaded_modules``), so after the user fixed the +plugin, the next load kept running the old, broken code until a restart. +Font registrations the failed instance made stayed listed too. +""" + +import json +import sys +from unittest.mock import MagicMock + +import pytest + +from src.plugin_system.plugin_manager import PluginManager +from src.plugin_system.plugin_state import PluginState + +PLUGIN_ID = "failed-load-demo" +MODULE_NAME = "plugin_failed_load_demo" + +_BROKEN_INIT = ''' +class Demo: + def __init__(self, plugin_id, config, display_manager, cache_manager, plugin_manager): + raise RuntimeError("broken constructor") +''' + +_BAD_CONFIG = ''' +class Demo: + VERSION = "bad-config" + def __init__(self, plugin_id, config, display_manager, cache_manager, plugin_manager): + pass + def validate_config(self): + return False +''' + +_FIXED = ''' +class Demo: + VERSION = "fixed" + def __init__(self, plugin_id, config, display_manager, cache_manager, plugin_manager): + self.enabled = True +''' + + +@pytest.fixture +def plugin_env(tmp_path): + plugins_dir = tmp_path / "plugins" + plugin_dir = plugins_dir / PLUGIN_ID + plugin_dir.mkdir(parents=True) + manifest = {"id": PLUGIN_ID, "name": "Demo", "class_name": "Demo", + "entry_point": "manager.py"} + (plugin_dir / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8") + manager = PluginManager(plugins_dir=str(plugins_dir)) + manager.font_manager = MagicMock() + manager.plugin_manifests[PLUGIN_ID] = manifest + yield manager, plugin_dir + sys.modules.pop(MODULE_NAME, None) + + +@pytest.mark.parametrize("first_source", [_BROKEN_INIT, _BAD_CONFIG], + ids=["instantiate-fails", "validate-config-fails"]) +def test_fixed_plugin_loads_new_code_after_failed_load(plugin_env, first_source): + pm, plugin_dir = plugin_env + (plugin_dir / "manager.py").write_text(first_source, encoding="utf-8") + + assert pm.load_plugin(PLUGIN_ID) is False + assert pm.state_manager.get_state(PLUGIN_ID) == PluginState.ERROR + assert MODULE_NAME not in sys.modules + assert PLUGIN_ID not in pm.plugin_loader._loaded_modules + pm.font_manager.forget_manager_fonts.assert_called_with(PLUGIN_ID) + + (plugin_dir / "manager.py").write_text(_FIXED, encoding="utf-8") + assert pm.load_plugin(PLUGIN_ID) is True + assert pm.plugins[PLUGIN_ID].VERSION == "fixed" diff --git a/test/test_plugin_system_utf8_reads.py b/test/test_plugin_system_utf8_reads.py new file mode 100644 index 00000000..88094728 --- /dev/null +++ b/test/test_plugin_system_utf8_reads.py @@ -0,0 +1,62 @@ +"""Plugin-system JSON files are read as UTF-8, whatever the locale says. + +store_manager (install_from_url's manifest, the secrets file) and +state_manager (plugin_state.json) opened text files without an encoding, so +the platform default applied. A manifest written in UTF-8 with a non-ASCII +name then read as mojibake -- or raised UnicodeDecodeError -- on a host +whose locale encoding is not UTF-8 (Windows' cp1252; a Pi with LANG=C). +On a UTF-8 host these pass either way; they fail on old code where the +default encoding differs. +""" + +import json + +import pytest + +from src.plugin_system.state_manager import PluginStateManager +from src.plugin_system.store_manager import PluginStoreManager + +NAME = "Météo Á" # "Á" is C3 81 in UTF-8; 0x81 is undefined in cp1252 + + +@pytest.fixture +def store(tmp_path, monkeypatch): + mgr = PluginStoreManager( + plugins_dir=str(tmp_path / "plugins"), + uninstalled_registry_path=str(tmp_path / "uninstalled.json")) + mgr.plugins_dir.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(mgr, "_install_dependencies", lambda *a, **k: True) + + def fake_clone(repo_url, target, branches): + target = type(mgr.plugins_dir)(target) + target.mkdir(parents=True, exist_ok=True) + manifest = {"id": "meteo", "name": NAME, "class_name": "P", + "display_modes": ["meteo"], "version": "1.0.0"} + (target / "manifest.json").write_bytes( + json.dumps(manifest, ensure_ascii=False).encode("utf-8")) + (target / "manager.py").write_text("class P: pass\n") + return "main" + + monkeypatch.setattr(mgr, "_install_via_git", fake_clone) + return mgr + + +def test_install_from_url_reads_a_utf8_manifest(store): + result = store.install_from_url("https://github.com/x/y") + assert result["success"] is True + assert result["name"] == NAME + written = json.loads( + (store.plugins_dir / "meteo" / "manifest.json").read_bytes().decode("utf-8")) + assert written["name"] == NAME + + +def test_state_manager_loads_a_utf8_state_file(tmp_path): + state_file = tmp_path / "plugin_state.json" + state_file.write_bytes(json.dumps({ + "version": 1, + "states": {"meteo": {"plugin_id": "meteo", "status": "installed", + "enabled": True, "metadata": {"label": NAME}}}, + }, ensure_ascii=False).encode("utf-8")) + + mgr = PluginStateManager(state_file=str(state_file)) + assert mgr.get_plugin_state("meteo").metadata["label"] == NAME diff --git a/test/test_resource_limits_validation.py b/test/test_resource_limits_validation.py new file mode 100644 index 00000000..213ed69a --- /dev/null +++ b/test/test_resource_limits_validation.py @@ -0,0 +1,81 @@ +"""Resource limits are validated before the monitor uses them. + +POST /plugins/limits/ built ResourceLimits straight from the request +JSON, and get_limits() did ``ResourceLimits(**cached)``. A dataclass does not +check its annotations, so ``{"max_execution_time": "5"}`` was stored as a +string and every later monitored update() of that plugin raised TypeError +comparing a float with it -- and an unknown key in the cached record raised +TypeError on load. +""" + +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from src.plugin_system.resource_monitor import ( # noqa: E402 + PluginResourceMonitor, +) +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + + +class SharedCache: + def __init__(self): + self.entries = {} + + def get(self, key, max_age=None, memory_ttl=None): + return self.entries.get(key) + + def set(self, key, value, *args, **kwargs): + self.entries[key] = value + + def delete(self, key): + self.entries.pop(key, None) + + +@pytest.fixture +def shared_cache(api_v3_module): + cache = SharedCache() + api_v3_module.api_v3.plugin_manager.resource_monitor = PluginResourceMonitor( + cache, enable_monitoring=False) + return cache + + +@pytest.mark.parametrize("body", [ + {"max_execution_time": "5"}, + {"max_memory_mb": -1}, + {"max_cpu_percent": True}, + {"warning_threshold": "high"}, + {"max_execution_time": [1]}, +]) +def test_post_rejects_bad_limits_with_400(api_v3_client, shared_cache, body): + resp = api_v3_client.post("/api/v3/plugins/limits/weather", json=body) + assert resp.status_code == 400 + assert resp.get_json()["status"] == "error" + assert "plugin_limits:weather" not in shared_cache.entries + + +def test_post_accepts_numbers_and_nulls(api_v3_client, shared_cache): + resp = api_v3_client.post("/api/v3/plugins/limits/weather", json={ + "max_memory_mb": 50, "max_cpu_percent": None, "max_execution_time": 5.0}) + assert resp.status_code == 200 + data = api_v3_client.get("/api/v3/plugins/limits/weather").get_json()["data"] + assert data == {"max_memory_mb": 50, "max_cpu_percent": None, + "max_execution_time": 5.0, "warning_threshold": 0.8} + + +@pytest.mark.parametrize("cached", [ + {"max_execution_time": "5"}, + {"max_execution_time": 5.0, "consecutive_failures": 3}, +]) +def test_bad_cached_limits_do_not_break_monitored_calls(cached): + cache = MagicMock() + cache.get.side_effect = lambda key, **kw: cached if key == "plugin_limits:p" else None + mon = PluginResourceMonitor(cache, enable_monitoring=False) + + # Unknown keys are dropped; a malformed value means "no limits". + assert mon.monitor_call("p", lambda: "ok") == "ok" + assert mon.monitor_call("p", lambda: "ok") == "ok" diff --git a/test/test_schema_path_resolution.py b/test/test_schema_path_resolution.py new file mode 100644 index 00000000..70188623 --- /dev/null +++ b/test/test_schema_path_resolution.py @@ -0,0 +1,68 @@ +"""SchemaManager.get_schema_path resolves plugin directories like the loader. + +It only tried ``/``, while the loader (plugin_dirs.py) also +finds a plugin by its manifest ``id`` and under ``ledmatrix-``. A plugin +installed under either of those loaded fine but had no settings form and was +never schema-validated. And every lookup of a plugin with no schema logged a +WARNING, once per call. +""" + +import json +import logging + +from src.plugin_system.schema_manager import SchemaManager + +SCHEMA = {"type": "object", "properties": {"enabled": {"type": "boolean"}}} + + +def _write_plugin(base, dir_name, plugin_id, schema=True): + plugin_dir = base / dir_name + plugin_dir.mkdir(parents=True) + (plugin_dir / "manifest.json").write_text(json.dumps({"id": plugin_id})) + if schema: + (plugin_dir / "config_schema.json").write_text(json.dumps(SCHEMA)) + return plugin_dir + + +def test_finds_schema_in_ledmatrix_prefixed_dir(tmp_path): + configured = tmp_path / "plugin-repos" + plugin_dir = _write_plugin(configured, "ledmatrix-stocks", "stocks") + sm = SchemaManager(plugins_dir=configured, project_root=tmp_path) + assert sm.get_schema_path("stocks") == plugin_dir / "config_schema.json" + + +def test_finds_schema_by_manifest_id(tmp_path): + configured = tmp_path / "plugin-repos" + plugin_dir = _write_plugin(configured, "some-other-name", "weather") + sm = SchemaManager(plugins_dir=configured, project_root=tmp_path) + assert sm.get_schema_path("weather") == plugin_dir / "config_schema.json" + + +def test_plugins_dir_still_wins_over_plugin_repos(tmp_path): + in_plugins = _write_plugin(tmp_path / "plugins", "ledmatrix-dupe", "dupe") + _write_plugin(tmp_path / "plugin-repos", "dupe", "dupe") + sm = SchemaManager(plugins_dir=None, project_root=tmp_path) + assert sm.get_schema_path("dupe") == in_plugins / "config_schema.json" + + +def test_miss_is_logged_once_at_debug_and_cached(tmp_path, caplog): + _write_plugin(tmp_path / "plugin-repos", "noschema", "noschema", schema=False) + sm = SchemaManager(plugins_dir=tmp_path / "plugin-repos", project_root=tmp_path, + logger=logging.getLogger("test_schema_path_resolution")) + with caplog.at_level(logging.DEBUG, logger="test_schema_path_resolution"): + for _ in range(3): + assert sm.get_schema_path("noschema") is None + misses = [r for r in caplog.records if "Schema file not found" in r.getMessage()] + assert len(misses) == 1 + assert misses[0].levelno == logging.DEBUG + + +def test_invalidate_cache_forgets_a_miss(tmp_path): + configured = tmp_path / "plugin-repos" + plugin_dir = _write_plugin(configured, "later", "later", schema=False) + sm = SchemaManager(plugins_dir=configured, project_root=tmp_path) + assert sm.get_schema_path("later") is None + + (plugin_dir / "config_schema.json").write_text(json.dumps(SCHEMA)) + sm.invalidate_cache("later") + assert sm.get_schema_path("later") == plugin_dir / "config_schema.json" diff --git a/test/test_store_install_from_url_rollback.py b/test/test_store_install_from_url_rollback.py new file mode 100644 index 00000000..aa20d982 --- /dev/null +++ b/test/test_store_install_from_url_rollback.py @@ -0,0 +1,86 @@ +"""install_from_url keeps the installed copy until the new one is in place. + +It deleted the existing plugin directory and then moved the download over it, +with no way back: a move that failed part-way left the user with no plugin at +all. And it did so outside the per-plugin reinstall lock install_plugin() +takes, so two overlapping installs of one id could interleave. +""" + +import json +import threading + +import pytest + +from src.plugin_system import store_manager as store_module +from src.plugin_system.plugin_dirs import BACKUP_MARKER +from src.plugin_system.store_manager import PluginStoreManager + +MANIFEST = { + "id": "demo", "name": "Demo", "class_name": "P", + "display_modes": ["demo"], "version": "2.0.0", +} + + +@pytest.fixture +def store(tmp_path, monkeypatch): + mgr = PluginStoreManager( + plugins_dir=str(tmp_path / "plugins"), + uninstalled_registry_path=str(tmp_path / "uninstalled.json")) + mgr.plugins_dir.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(mgr, "_install_dependencies", lambda *a, **k: True) + + def fake_clone(repo_url, target, branches): + target = type(mgr.plugins_dir)(target) + target.mkdir(parents=True, exist_ok=True) + (target / "manifest.json").write_text(json.dumps(MANIFEST)) + (target / "manager.py").write_text("NEW = True\n") + return "main" + + monkeypatch.setattr(mgr, "_install_via_git", fake_clone) + old = mgr.plugins_dir / "demo" + old.mkdir() + (old / "manager.py").write_text("OLD = True\n") + return mgr + + +def _leftover_backups(store): + return [p for p in store.plugins_dir.iterdir() if BACKUP_MARKER in p.name] + + +def test_failed_move_restores_the_existing_install(store, monkeypatch): + def broken_move(src, dst): + raise OSError("disk full") + + monkeypatch.setattr(store_module.shutil, "move", broken_move) + + result = store.install_from_url("https://github.com/x/y") + + assert result["success"] is False + assert (store.plugins_dir / "demo" / "manager.py").read_text() == "OLD = True\n" + assert _leftover_backups(store) == [] + + +def test_successful_replace_leaves_no_backup(store): + result = store.install_from_url("https://github.com/x/y") + + assert result["success"] is True + assert (store.plugins_dir / "demo" / "manager.py").read_text() == "NEW = True\n" + assert _leftover_backups(store) == [] + + +def test_replace_happens_under_the_reinstall_lock(store, monkeypatch): + real_move = store_module.shutil.move + seen = {} + + def spying_move(src, dst): + lock = store._get_reinstall_lock("demo") + probe = threading.Thread( + target=lambda: seen.setdefault("free", lock.acquire(blocking=False))) + probe.start() + probe.join() + return real_move(src, dst) + + monkeypatch.setattr(store_module.shutil, "move", spying_move) + + assert store.install_from_url("https://github.com/x/y")["success"] is True + assert seen == {"free": False} diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index 16d753f8..310a783f 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -234,7 +234,13 @@ def get_plugin_health_single(plugin_id): }) @api_v3.route('/plugins/health//reset', methods=['POST']) def reset_plugin_health(plugin_id): - """Reset health state for a plugin (manual recovery)""" + """Reset health state for a plugin (manual recovery). + + This resets the web process's tracker and the persisted record. The + display service runs its own tracker in another process and keeps its + in-memory state, so its next recorded success or failure can write that + state back; restart the display service for a reset it will honour. + """ if not api_v3.plugin_manager: return jsonify({'status': 'error', 'message': 'Plugin manager not initialized'}), 500 @@ -307,7 +313,12 @@ def get_plugin_metrics_single(plugin_id): }) @api_v3.route('/plugins/metrics//reset', methods=['POST']) def reset_plugin_metrics(plugin_id): - """Reset metrics for a plugin""" + """Reset metrics for a plugin. + + Only the web process's copy and the persisted snapshot are cleared. The + display service keeps accumulating in its own process and republishes + its totals on its next persist, so the reset does not stick while it runs. + """ if not api_v3.plugin_manager: return jsonify({'status': 'error', 'message': 'Plugin manager not initialized'}), 500 @@ -326,7 +337,13 @@ def reset_plugin_metrics(plugin_id): }) @api_v3.route('/plugins/limits/', methods=['GET', 'POST']) def manage_plugin_limits(plugin_id): - """Get or set resource limits for a plugin""" + """Get or set resource limits for a plugin. + + A POST updates the web process's monitor and the persisted record. The + display service reads persisted limits only until it has some for a + plugin, so a change to existing limits takes effect there after the + display service restarts. + """ if not api_v3.plugin_manager: return jsonify({'status': 'error', 'message': 'Plugin manager not initialized'}), 500 @@ -358,14 +375,18 @@ def manage_plugin_limits(plugin_id): else: # POST - Set limits data = request.get_json(silent=True) or {} - from src.plugin_system.resource_monitor import ResourceLimits + from src.plugin_system.resource_monitor import invalid_limit_field, limits_from_dict - limits = ResourceLimits( - max_memory_mb=data.get('max_memory_mb'), - max_cpu_percent=data.get('max_cpu_percent'), - max_execution_time=data.get('max_execution_time'), - warning_threshold=data.get('warning_threshold', 0.8) - ) + # Validate here: a string limit stored as-is made every later update + # of the plugin raise TypeError inside the resource monitor. The + # message is built from the field name, not from an exception. + bad = invalid_limit_field(data) + if bad == 'limits': + return jsonify({'status': 'error', 'message': 'Limits must be a JSON object'}), 400 + if bad: + return jsonify({'status': 'error', + 'message': f'{bad} must be a non-negative number or null'}), 400 + limits = limits_from_dict(data) api_v3.plugin_manager.resource_monitor.set_limits(plugin_id, limits)