diff --git a/.gitignore b/.gitignore index fdfaa90b..641d7c44 100644 --- a/.gitignore +++ b/.gitignore @@ -53,3 +53,25 @@ skin_renders/ # JS test deps (test/js) node_modules/ package-lock.json + +# Team logos fetched at runtime. +# +# src/logo_downloader.py and LogoHelper write into assets/sports/_logos/ +# whenever a plugin meets a team whose logo is not on disk. Those directories are +# also tracked -- 209 NCAA logos and 153 soccer ones ship with the repo -- so +# every rig accumulated untracked files it was never meant to commit and +# `git status` was permanently dirty. That noise is not harmless: it trains +# everyone to ignore the one signal that says a checkout is not what you think +# it is, which is how a stale tree sat unnoticed on a rig until a restart +# surfaced four dead sports plugins. +# +# Ignoring a directory does not untrack what is already in it, so the logos that +# ship keep shipping. Only new downloads are hidden. +# +# Adding a logo on purpose is rare and deliberate -- the last time was #415, four +# named NCAA logos a plugin needed, and there has been no other in a year. Do it +# with an explicit override: +# git add -f assets/sports/ncaa_logos/DUKE.png +assets/sports/*_logos/ +assets/stocks/ticker_icons/ +assets/stocks/crypto_icons/ diff --git a/scripts/install/install_service.sh b/scripts/install/install_service.sh index cd4144cc..0a7955d8 100755 --- a/scripts/install/install_service.sh +++ b/scripts/install/install_service.sh @@ -16,20 +16,39 @@ USER_HOME=$(eval echo ~$ACTUAL_USER) # Determine the Project Root Directory (parent of scripts/install/) PROJECT_ROOT_DIR=$(cd "$(dirname "$0")/../.." && pwd) +# shellcheck source=scripts/install/lib_systemd_render.sh +source "$PROJECT_ROOT_DIR/scripts/install/lib_systemd_render.sh" + echo "Installing LED Matrix Display Service for user: $ACTUAL_USER" echo "Using home directory: $USER_HOME" echo "Project root directory: $PROJECT_ROOT_DIR" -# Create a temporary service file for the main display with the correct paths -# Assuming ledmatrix.service template exists and uses /home/ledpi as a placeholder for user home +# Render the main display unit from its template. The display service runs as +# root (it needs GPIO), so __USER__ is always root here -- unlike the web unit +# below, which runs as whoever installed it. +# +# A missing template or a failed render is fatal: falling through would leave +# whatever unit already sits at /etc/systemd/system/ledmatrix.service (from a +# previous install) untouched, and the enable/start step below would then +# silently reuse that stale unit instead of the one this run was asked to +# install. if [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" ]; then - sed "s|/home/ledpi|$USER_HOME|g; s|__PROJECT_ROOT_DIR__|$PROJECT_ROOT_DIR|g; s|__USER__|root|g" "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" > /tmp/ledmatrix.service.tmp + ESCAPED_PROJECT_ROOT_DIR=$(sed_escape_replacement "$PROJECT_ROOT_DIR") + MAIN_UNIT_TMP=$(mktemp) + trap 'rm -f "$MAIN_UNIT_TMP"' EXIT + if ! sed "s|__PROJECT_ROOT_DIR__|$ESCAPED_PROJECT_ROOT_DIR|g; s|__USER__|root|g" \ + "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" > "$MAIN_UNIT_TMP"; then + echo "ERROR: failed to render ledmatrix.service from its template." >&2 + exit 1 + fi # Copy the service file to the systemd directory - sudo cp /tmp/ledmatrix.service.tmp /etc/systemd/system/ledmatrix.service + sudo cp "$MAIN_UNIT_TMP" /etc/systemd/system/ledmatrix.service # Clean up - rm /tmp/ledmatrix.service.tmp + rm -f "$MAIN_UNIT_TMP" + trap - EXIT else - echo "WARNING: ledmatrix.service template not found at $PROJECT_ROOT_DIR/systemd/ledmatrix.service. Main display service not configured." + echo "ERROR: ledmatrix.service template not found at $PROJECT_ROOT_DIR/systemd/ledmatrix.service." >&2 + exit 1 fi @@ -48,42 +67,49 @@ fi # === LEDMatrix Web Interface service (ledmatrix-web.service) === echo "Installing LEDMatrix Web Interface service (ledmatrix-web.service)..." -WEB_SERVICE_FILE_CONTENT=$(cat < /dev/null +# Rendered from systemd/ledmatrix-web.service, the same template +# install_web_service.sh uses. This was an inline heredoc until it drifted from +# the template: it had lost Wants=network-online.target, RestartSec, +# SyslogIdentifier, CacheDirectory and Environment=USE_THREADING. Because +# src/startup_validator.py compares the installed unit against the template, +# every boot warned "re-run install_service.sh" -- and doing so reinstalled the +# same stale copy, so the warning could never clear. +# +# As with the main unit above, a missing template or a failed render is +# fatal -- otherwise the enable/start check below would fall back to +# whatever unit (possibly stale) already exists at the destination path. +if [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service" ]; then + ESCAPED_ACTUAL_USER=$(sed_escape_replacement "$ACTUAL_USER") + WEB_UNIT_TMP=$(mktemp) + trap 'rm -f "$WEB_UNIT_TMP"' EXIT + if ! sed "s|__PROJECT_ROOT_DIR__|$ESCAPED_PROJECT_ROOT_DIR|g; s|__USER__|$ESCAPED_ACTUAL_USER|g" \ + "$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service" > "$WEB_UNIT_TMP"; then + echo "ERROR: failed to render ledmatrix-web.service from its template." >&2 + exit 1 + fi + sudo cp "$WEB_UNIT_TMP" /etc/systemd/system/ledmatrix-web.service + rm -f "$WEB_UNIT_TMP" + trap - EXIT +else + echo "ERROR: ledmatrix-web.service template not found at $PROJECT_ROOT_DIR/systemd/ledmatrix-web.service." >&2 + exit 1 +fi echo "Reloading systemd daemon for web service..." sudo systemctl daemon-reload -echo "Enabling ledmatrix-web.service to start on boot..." -sudo systemctl enable ledmatrix-web.service +if [ -f "/etc/systemd/system/ledmatrix-web.service" ]; then + echo "Enabling ledmatrix-web.service to start on boot..." + sudo systemctl enable ledmatrix-web.service -echo "Starting ledmatrix-web.service..." -sudo systemctl start ledmatrix-web.service + echo "Starting ledmatrix-web.service..." + sudo systemctl start ledmatrix-web.service -echo "LEDMatrix Web Interface service (ledmatrix-web.service) installation complete." -echo "It will start based on the 'web_display_autostart' setting in config/config.json." + echo "LEDMatrix Web Interface service (ledmatrix-web.service) installation complete." + echo "It will start based on the 'web_display_autostart' setting in config/config.json." +else + echo "Skipping enable/start for ledmatrix-web.service as it was not configured." +fi # === End of LEDMatrix Web Interface service === diff --git a/scripts/install/install_web_service.sh b/scripts/install/install_web_service.sh index 6df16d71..1e7a9476 100755 --- a/scripts/install/install_web_service.sh +++ b/scripts/install/install_web_service.sh @@ -17,6 +17,9 @@ fi # Determine the Project Root Directory (parent of scripts/install/) PROJECT_ROOT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd) +# shellcheck source=scripts/install/lib_systemd_render.sh +source "$PROJECT_ROOT_DIR/scripts/install/lib_systemd_render.sh" + echo "Installing for user: $ACTUAL_USER" echo "Project root directory: $PROJECT_ROOT_DIR" @@ -26,37 +29,22 @@ if [ "$EUID" -ne 0 ]; then exit 1 fi -# Generate the service file dynamically with the correct paths -echo "Generating service file with dynamic paths..." -WEB_SERVICE_FILE_CONTENT=$(cat < /etc/systemd/system/ledmatrix-web.service +ESCAPED_PROJECT_ROOT_DIR=$(sed_escape_replacement "$PROJECT_ROOT_DIR") +ESCAPED_ACTUAL_USER=$(sed_escape_replacement "$ACTUAL_USER") +sed "s|__PROJECT_ROOT_DIR__|$ESCAPED_PROJECT_ROOT_DIR|g; s|__USER__|$ESCAPED_ACTUAL_USER|g" \ + "$TEMPLATE" > /etc/systemd/system/ledmatrix-web.service # Ensure cache directory exists with proper permissions # This is a fallback for older systemd versions that don't support CacheDirectory diff --git a/scripts/install/install_wifi_monitor.sh b/scripts/install/install_wifi_monitor.sh index 5d224c03..0c24b0b8 100755 --- a/scripts/install/install_wifi_monitor.sh +++ b/scripts/install/install_wifi_monitor.sh @@ -18,6 +18,9 @@ USER_HOME=$(eval echo ~$ACTUAL_USER) # Determine the Project Root Directory (parent of scripts/install/) PROJECT_ROOT_DIR=$(cd "$(dirname "$0")/../.." && pwd) +# shellcheck source=scripts/install/lib_systemd_render.sh +source "$PROJECT_ROOT_DIR/scripts/install/lib_systemd_render.sh" + echo "Installing LED Matrix WiFi Monitor Service for user: $ACTUAL_USER" echo "Using home directory: $USER_HOME" echo "Project root directory: $PROJECT_ROOT_DIR" @@ -64,30 +67,19 @@ if [ ${#MISSING_PACKAGES[@]} -gt 0 ]; then echo "✓ Package installation completed" fi -# Create service file with correct paths +# Render the unit from systemd/ledmatrix-wifi-monitor.service rather than +# inlining a second copy here. The copy this replaced had already drifted -- +# it wrote StandardOutput/StandardError=syslog where the template says journal. echo "" echo "Creating systemd service file..." -SERVICE_FILE_CONTENT=$(cat < /dev/null diff --git a/scripts/install/lib_systemd_render.sh b/scripts/install/lib_systemd_render.sh new file mode 100644 index 00000000..d07dcec1 --- /dev/null +++ b/scripts/install/lib_systemd_render.sh @@ -0,0 +1,27 @@ +#!/bin/bash +# +# Shared helper for rendering systemd unit templates via sed. +# +# Sourced by install_service.sh, install_web_service.sh and +# install_wifi_monitor.sh so all three escape sed replacement text the same +# way instead of carrying three copies of the same fix. + +# sed_escape_replacement VALUE +# +# Print VALUE escaped for safe use as the replacement side of `sed +# s|pattern|replacement|`. Every one of these scripts builds its sed +# expression by interpolating a shell variable (a path, a username, ...) +# straight into the replacement text. sed gives three characters special +# meaning there: backslash (escape character), & (whole match) and the +# delimiter itself (here `|`). A value containing any of them -- e.g. a +# username or path with an `&`, a literal backslash, or a `|` -- would +# otherwise corrupt the rendered unit file instead of being substituted +# literally. Escape the backslash first so the later escapes aren't +# double-escaped. +sed_escape_replacement() { + local value="$1" + value="${value//\\/\\\\}" + value="${value//&/\\&}" + value="${value//|/\\|}" + printf '%s' "$value" +} diff --git a/scripts/utils/start_web_conditionally.py b/scripts/utils/start_web_conditionally.py index b3c3c99d..b1492ac0 100644 --- a/scripts/utils/start_web_conditionally.py +++ b/scripts/utils/start_web_conditionally.py @@ -74,23 +74,41 @@ def install_dependencies(): print(f"Failed to install dependencies: {e}") return False +#: String spellings that turn autostart OFF. Anything else -- including the key +#: being absent entirely -- leaves it on. +DISABLED_STRINGS = ("off", "false", "no", "0") + + +def autostart_enabled(config_data): + """Whether to bring the web interface up. Defaults to True. + + config.template.json and first_time_install.sh both ship + ``web_display_autostart`` as true, so a config that lacks the key is an + older or hand-edited one rather than a request to stay down. Defaulting to + False meant any such config silently got no web interface -- and because + the "not starting" path exits 0, systemd reported the unit as successfully + started while nothing was listening. Only an explicit false/off disables it. + """ + value = config_data.get("web_display_autostart", True) + if isinstance(value, str): + return value.strip().lower() not in DISABLED_STRINGS + return bool(value) + + def main(): try: with open(CONFIG_FILE, 'r') as f: config_data = json.load(f) except FileNotFoundError: - print(f"Config file {CONFIG_FILE} not found. Web interface will not start.") - sys.exit(0) # Exit gracefully, don't start - except Exception as e: - print(f"Error reading config file {CONFIG_FILE}: {e}. Web interface will not start.") - sys.exit(1) # Exit with error, service might restart depending on config + # The web interface is how a config gets created and repaired, so a + # missing one is the case where the user needs it most. + print(f"Config file {CONFIG_FILE} not found. Starting the web interface so it can be configured.") + config_data = {} + except (json.JSONDecodeError, OSError) as e: + print(f"Error reading config file {CONFIG_FILE}: {e}. Starting the web interface anyway so the config can be repaired.") + config_data = {} - autostart_enabled = config_data.get("web_display_autostart", False) - - # Handle both boolean True and string "on"/"true" values - is_enabled = (autostart_enabled is True) or (isinstance(autostart_enabled, str) and autostart_enabled.lower() in ("on", "true", "yes", "1")) - - if is_enabled: + if autostart_enabled(config_data): print("Configuration 'web_display_autostart' is enabled. Starting web interface...") # Only install dependencies if not already done during first-time setup @@ -116,7 +134,7 @@ def main(): print(f"Failed to exec web interface: {e}") sys.exit(1) # Failed to start else: - print("Configuration 'web_display_autostart' is false or not set. Web interface will not be started.") + print("Configuration 'web_display_autostart' is explicitly disabled. Web interface will not be started.") sys.exit(0) # Exit gracefully, service considered successful if __name__ == '__main__': diff --git a/src/base_classes/baseball.py b/src/base_classes/baseball.py index ac84bfb7..f36ca9db 100644 --- a/src/base_classes/baseball.py +++ b/src/base_classes/baseball.py @@ -37,51 +37,6 @@ class Baseball(SportsCore): self.data_source = ESPNDataSource(logger) self.sport = "baseball" - def _get_baseball_display_text(self, game: Dict) -> str: - """Get baseball-specific display text.""" - try: - display_parts = [] - - # Inning information - if self.show_innings: - inning = game.get("inning", "") - if inning: - display_parts.append(f"Inning: {inning}") - - # Outs information - if self.show_outs: - outs = game.get("outs", 0) - if outs is not None: - display_parts.append(f"Outs: {outs}") - - # Bases information - if self.show_bases: - bases = game.get("bases", "") - if bases: - display_parts.append(f"Bases: {bases}") - - # Count information - if self.show_count: - strikes = game.get("strikes", 0) - balls = game.get("balls", 0) - if strikes is not None and balls is not None: - display_parts.append(f"Count: {balls}-{strikes}") - - # Pitcher/Batter information - if self.show_pitcher_batter: - pitcher = game.get("pitcher", "") - batter = game.get("batter", "") - if pitcher: - display_parts.append(f"Pitcher: {pitcher}") - if batter: - display_parts.append(f"Batter: {batter}") - - return " | ".join(display_parts) if display_parts else "" - - except Exception as e: - self.logger.error(f"Error getting baseball display text: {e}") - return "" - def _is_baseball_game_live(self, game: Dict) -> bool: """Check if a baseball game is currently live.""" try: diff --git a/src/base_classes/sports/core.py b/src/base_classes/sports/core.py index 251ba5cb..452e9bcc 100644 --- a/src/base_classes/sports/core.py +++ b/src/base_classes/sports/core.py @@ -8,6 +8,7 @@ import os import tempfile import time from abc import ABC, abstractmethod +from collections import OrderedDict from datetime import datetime, timedelta from pathlib import Path from typing import Any, Dict, List, Optional, Tuple @@ -167,7 +168,14 @@ class SportsCore(ABC): self.session.mount("https://", adapter) self.session.mount("http://", adapter) - self._logo_cache = {} + # LRU-bounded: entries are decoded RGBA thumbnails, not file bytes. + # Each is up to display_width*1.5 x display_height*1.5 -- about 36KB on + # a 256x64 panel, more for wide wordmarks. The key is a team + # abbreviation and assets/sports/ncaa_logos alone ships 307 of them, so + # an unbounded dict here held the whole league: ~11-18MB per manager + # instance, and a league runs three (live/recent/upcoming) each with + # its own cache. That is real money on a 1GB Pi. + self._logo_cache: "OrderedDict[str, Image.Image]" = OrderedDict() # Font caches for _load_custom_font_from_element_config: per-frame # callers (font-ladder walks) resolve the same (name, size) over and @@ -560,11 +568,17 @@ class SportsCore(ABC): draw.text((x + dx, y + dy), text, font=font, fill=outline_color) draw.text((x, y), text, font=font, fill=fill) + #: Decoded logos to keep. A scroll of "other games" shows on the order of + #: 20 games (40 teams), so this holds a full cycle without thrashing while + #: capping the cache well below a 307-team league. + _LOGO_CACHE_MAX = 64 + def _load_and_resize_logo(self, team_id: str, team_abbrev: str, logo_path: Path, logo_url: str | None ) -> Optional[Image.Image]: """Load and resize a team logo, with caching and automatic download if missing.""" self.logger.debug(f"Logo path: {logo_path}") if team_abbrev in self._logo_cache: self.logger.debug(f"Using cached logo for {team_abbrev}") + self._logo_cache.move_to_end(team_abbrev) return self._logo_cache[team_abbrev] try: @@ -604,6 +618,8 @@ class SportsCore(ABC): max_height = int(self.display_height * 1.5) logo.thumbnail((max_width, max_height), Image.Resampling.LANCZOS) self._logo_cache[team_abbrev] = logo + while len(self._logo_cache) > self._LOGO_CACHE_MAX: + self._logo_cache.popitem(last=False) return logo except Exception as e: diff --git a/src/common/logo_helper.py b/src/common/logo_helper.py index a64ca340..0361a08b 100644 --- a/src/common/logo_helper.py +++ b/src/common/logo_helper.py @@ -8,6 +8,7 @@ Extracted from LEDMatrix core to provide reusable functionality for plugins. import logging import os import tempfile +import time from pathlib import Path from typing import Dict, List, Optional, Union @@ -20,6 +21,21 @@ from src.common.permission_utils import ( get_assets_file_mode ) +# How long a missing logo stays remembered as missing. +# +# This was 600s, and measured on a live rig that turned out to suppress nothing: +# the display rotation is ~618s, so every recheck landed just as the plugin came +# round again and the warning rate was unchanged at ~6/hour. A TTL has to be long +# relative to the loop that does the asking, not merely "a while". +# +# An hour is safe because the TTL is not the main way an entry clears. A download +# through load_logo_with_download() drops it immediately, and clear_cache() drops +# all of them; the TTL only covers a file that appeared some other way -- someone +# copying one in by hand. Waiting up to an hour for that, or restarting, is a fair +# trade for not re-warning about a file nobody is going to add. +MISSING_LOGO_RECHECK_SECONDS = 3600.0 + + # Well above any real team logo; bounds what a remote URL can write to disk. MAX_LOGO_BYTES = 10 * 1024 * 1024 @@ -56,6 +72,14 @@ class LogoHelper: # In-memory logo cache self._logo_cache: Dict[str, Image.Image] = {} self._cache_order: List[str] = [] # For LRU cache management + + # Misses, so an absent file is stat'd and warned about once rather than + # on every call. Without this a permanently missing logo produced a + # warning per rotation forever -- measured at 114 lines in 24 hours for + # a single missing ticker icon, for a file nobody was going to add. + # Time-bounded rather than permanent so a logo that appears later (the + # downloader writes them at runtime) is still picked up. + self._missing_logos: Dict[str, float] = {} # Session for HTTP requests self.session = requests.Session() @@ -100,9 +124,19 @@ class LogoHelper: self._cache_order.append(cache_key) return self._logo_cache[cache_key] + # A known-missing file: skip the stat and stay quiet until the entry + # ages out. Checked after the positive cache so a logo that has since + # been loaded always wins. + missed_at = self._missing_logos.get(cache_key) + if missed_at is not None: + if time.time() - missed_at < MISSING_LOGO_RECHECK_SECONDS: + return None + del self._missing_logos[cache_key] + try: logo_path = Path(logo_path) if not logo_path.exists(): + self._missing_logos[cache_key] = time.time() self.logger.warning(f"Logo not found for {team_abbr} at {logo_path}") return None @@ -179,6 +213,11 @@ class LogoHelper: self._logo_cache.pop(key, None) if key in self._cache_order: self._cache_order.remove(key) + # The file exists now, so any record of it being missing is wrong -- + # and load_logo() consults that record before it stats the disk, so + # leaving it would hide a logo we just downloaded. + for key in [k for k in self._missing_logos if k.startswith(prefix)]: + del self._missing_logos[key] @staticmethod def _refresh_stale_placeholder(logo_path: Path) -> None: @@ -270,6 +309,7 @@ class LogoHelper: """Clear the logo cache.""" self._logo_cache.clear() self._cache_order.clear() + self._missing_logos.clear() self.logger.debug("Logo cache cleared") def get_cache_stats(self) -> Dict[str, int]: diff --git a/src/display_controller.py b/src/display_controller.py index 8a902570..44875f63 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -2169,7 +2169,14 @@ class DisplayController: types.SimpleNamespace(display=_display_target), plugin_id, force_clear=self.force_change, - display_mode=active_mode if _accepts_display_mode else None + display_mode=active_mode if _accepts_display_mode else None, + # Already resolved and cached above. + # Without this the executor re-derives + # it with inspect.signature() against + # the SimpleNamespace built two lines + # up -- a fresh callable every call, so + # nothing there can ever cache. + accepts_display_mode=_accepts_display_mode ) except Exception: # pragma: no cover - defensive; # execute_display catches everything @@ -2492,6 +2499,15 @@ class DisplayController: 1.0 / display_interval ) + # Deliberate: frames after the first call + # display() directly rather than through + # PluginExecutor. The executor spawns a thread per + # call, which at this loop's frame rate would cost + # more than the advisory timeout it buys -- and + # that timeout cannot cancel a hung plugin anyway + # (see execute_with_timeout). The first dispatch + # above still goes through it, so load-time + # failures are still caught and recorded. while True: _frame_start = time.perf_counter() try: @@ -2564,6 +2580,15 @@ class DisplayController: display_interval ) + # Deliberate: frames after the first call + # display() directly rather than through + # PluginExecutor. The executor spawns a thread per + # call, which at this loop's frame rate would cost + # more than the advisory timeout it buys -- and + # that timeout cannot cancel a hung plugin anyway + # (see execute_with_timeout). The first dispatch + # above still goes through it, so load-time + # failures are still caught and recorded. while True: time.sleep(display_interval) self._tick_plugin_updates() diff --git a/src/plugin_system/base_plugin.py b/src/plugin_system/base_plugin.py index dbe14a49..54aa7426 100644 --- a/src/plugin_system/base_plugin.py +++ b/src/plugin_system/base_plugin.py @@ -520,6 +520,38 @@ class BasePlugin(ABC): """ return + def get_update_interval(self) -> Optional[float]: + """ + How often this plugin wants update() called, right now, in seconds. + + The manifest's ``update_interval`` is a single static number, which + cannot say "poll me every 15 seconds while a game is in progress and + every 15 minutes when nothing is on". Only the plugin knows which is + true at any moment, so override this to say so. + + Return None (the default) to accept the manifest/config value. + + Two constraints, both because the scheduler calls this on every tick of + the render loop: + + - It must be cheap. Attribute reads only -- no config lookups, no I/O, + no locks that a fetch might be holding. + - It must not raise. A raising hook is ignored and the static interval + used, but a hook that raises every tick also logs every tick. + + Values below PluginManager.MIN_DYNAMIC_UPDATE_INTERVAL are clamped up: + a plugin asking for 0 would otherwise busy-wait against its own API. + + Example:: + + def get_update_interval(self): + # Fast while something is actually live, manifest default otherwise. + if any(m.live_games for m in self._live_managers): + return self.config.get("live_update_interval", 15) + return None + """ + return None + def has_live_priority(self) -> bool: """ Check if this plugin has live priority enabled. diff --git a/src/plugin_system/plugin_executor.py b/src/plugin_system/plugin_executor.py index 2fac5c5e..c1a23b49 100644 --- a/src/plugin_system/plugin_executor.py +++ b/src/plugin_system/plugin_executor.py @@ -76,6 +76,13 @@ class PluginExecutor: thread.start() thread.join(timeout=timeout) + # NB: this timeout is advisory. Nothing cancels the thread -- Python + # has no way to -- so on expiry the operation keeps running to + # completion in the background and only this caller gives up waiting. + # A plugin that hangs permanently leaks one daemon thread per attempt. + # Callers that hold a resource across the call must release it from + # inside the wrapped callable rather than after this returns; see the + # _release_display_lock guard inside DisplayController.run(). if not result_container['completed']: error_msg = f"{plugin_context} operation timed out after {timeout}s" self.logger.error(error_msg) @@ -148,7 +155,8 @@ class PluginExecutor: plugin_id: str, force_clear: bool = False, display_mode: Optional[str] = None, - timeout: Optional[float] = None + timeout: Optional[float] = None, + accepts_display_mode: Optional[bool] = None ) -> bool: """ Execute plugin display() method with error handling. @@ -159,6 +167,9 @@ class PluginExecutor: force_clear: Whether to force clear display display_mode: Optional display mode parameter timeout: Timeout in seconds (None = use default) + accepts_display_mode: Whether plugin.display() takes a + display_mode keyword. Pass it when the caller already knows; + None falls back to inspecting the callable. Returns: True if display succeeded, False otherwise @@ -166,10 +177,20 @@ class PluginExecutor: try: start_time = time.time() - # Check if plugin accepts display_mode parameter - import inspect - sig = inspect.signature(plugin.display) - has_display_mode = 'display_mode' in sig.parameters + # Does display() take a display_mode keyword? The caller usually + # knows and caches the answer, so prefer what it passed. + # + # Inspecting here was not merely redundant, it could never be + # cached: display_controller wraps the real plugin in a fresh + # SimpleNamespace per call, so inspect.signature() saw a new + # callable every time and paid ~55us on a Pi 4 to re-derive a + # value the caller had computed one line earlier and stored in + # self._plugin_accepts_display_mode. + if accepts_display_mode is None: + import inspect + accepts_display_mode = ( + 'display_mode' in inspect.signature(plugin.display).parameters) + has_display_mode = accepts_display_mode # Capture the return value from the plugin's display() method if has_display_mode and display_mode: diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 0dc7a428..2c8de821 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -8,6 +8,7 @@ API Version: 1.0.0 """ import json +import math import queue import sys import time @@ -779,15 +780,68 @@ class PluginManager: return None + def _dynamic_update_interval(self, plugin_id: str, plugin_instance: Any) -> Optional[float]: + """The interval a plugin asks for right now, or None if it has no view.""" + hook = getattr(plugin_instance, 'get_update_interval', None) + if not callable(hook): + return None + try: + requested = hook() + except Exception as exc: # pylint: disable=broad-except + self.logger.debug( + "get_update_interval() failed for %s, using the static interval: %s", + plugin_id, exc) + return None + if requested is None: + return None + if isinstance(requested, bool): + self.logger.debug( + "get_update_interval() returned a bool for %s, which is not a number", + plugin_id) + return None + try: + requested = float(requested) + except (TypeError, ValueError): + self.logger.debug( + "get_update_interval() returned %r for %s, which is not a number", + requested, plugin_id) + return None + if not math.isfinite(requested): # NaN / +inf / -inf + return None + return max(requested, self.MIN_DYNAMIC_UPDATE_INTERVAL) + + #: Floor for a plugin-requested interval. A plugin asking for 0 (or a + #: negative) would otherwise be re-entered on every tick of the render + #: loop, which is a busy-wait against whatever API it fetches. + MIN_DYNAMIC_UPDATE_INTERVAL = 5.0 + def _get_plugin_update_interval(self, plugin_id: str, plugin_instance: Any) -> Optional[float]: """ Get the data-fetch interval for a plugin (seconds between update() calls). - Result is cached per plugin_id after the first lookup to avoid calling - config_manager.get_config() — which returns a full dict copy — on every - tick of the 30-fps display loop. The cache is invalidated when a plugin - is loaded or unloaded. + A plugin may implement ``get_update_interval()`` to vary its own cadence + at runtime, which the static manifest value cannot express. The case + this exists for: a sports scoreboard needs to poll every 15s while a + game is in progress and every 15 minutes when nothing is on, and only + the plugin knows which is true right now. Returning None from the hook + means "no opinion", and the static resolution below applies. + + The hook is called on every scheduling tick, so implementations must be + cheap — attribute reads, no config lookups and no I/O. A raising or + non-numeric hook is ignored rather than allowed to stop the plugin + updating, since a scheduler that propagates a plugin bug stops every + other plugin too. + + The static result is cached per plugin_id after the first lookup to + avoid calling config_manager.get_config() — which returns a full dict + copy — on every tick of the 30-fps display loop. The cache is + invalidated when a plugin is loaded or unloaded. The dynamic hook is + deliberately *not* cached: caching it would defeat its only purpose. """ + dynamic = self._dynamic_update_interval(plugin_id, plugin_instance) + if dynamic is not None: + return dynamic + if plugin_id in self._update_interval_cache: return self._update_interval_cache[plugin_id] diff --git a/src/plugin_system/state_reconciliation.py b/src/plugin_system/state_reconciliation.py index afbd2e1d..e967fbdf 100644 --- a/src/plugin_system/state_reconciliation.py +++ b/src/plugin_system/state_reconciliation.py @@ -8,6 +8,7 @@ Detects and fixes inconsistencies between: - State manager state """ +import json from typing import Dict, Any, List, Set from dataclasses import dataclass from enum import Enum @@ -55,6 +56,35 @@ class ReconciliationResult: message: str +def still_unresolved(entries: List[Dict[str, Any]], + config_keys: Set[str], + installed_ids: Set[str]) -> List[Dict[str, Any]]: + """Drop stored reconciliation findings that are no longer true. + + The verdict is written once to a status file and served to the web UI from + there, and a run that fails to apply a fix also declares it "will not retry + automatically". Together those froze a single moment forever: a device whose + plugins were all present in config kept being told, for hours, that four of + them were missing and should be removed from config.json. + + Entry kinds this cannot re-check are kept, so filtering only ever removes + findings that are provably stale. + """ + live: List[Dict[str, Any]] = [] + for entry in entries: + kind = entry.get('type') + plugin_id = entry.get('plugin_id') + if kind == InconsistencyType.PLUGIN_MISSING_IN_CONFIG.value: + if plugin_id not in config_keys: + live.append(entry) + elif kind == InconsistencyType.PLUGIN_MISSING_ON_DISK.value: + if plugin_id not in installed_ids: + live.append(entry) + else: + live.append(entry) + return live + + class StateReconciliation: """ State reconciliation system. @@ -200,15 +230,37 @@ class StateReconciliation: 'github', 'youtube', }) + def _secrets_top_level_keys(self) -> Set[str]: + """Top-level keys that load_config() merges in from the secrets file. + + load_config() merges config_secrets.json into the config it returns, so + those keys sit alongside plugin ids. _SYSTEM_CONFIG_KEYS named them + individually ('github', 'youtube'), which broke the moment anything else + was written there: a 'data' key became a phantom plugin, permanently + reported as "in config but not on disk". Reading the file keeps this + correct no matter what it holds. + """ + try: + path = self.config_manager.get_secrets_path() + with open(path, 'r') as f: + secrets = json.load(f) + except (AttributeError, OSError, TypeError, ValueError): + # Deliberately broad: an unreadable, absent, malformed or + # non-path secrets location must narrow this set, never break + # reconciliation. ValueError covers json.JSONDecodeError. + return set() + return set(secrets) if isinstance(secrets, dict) else set() + def _get_config_state(self) -> Dict[str, Dict[str, Any]]: """Get plugin state from config file.""" state = {} try: config = self.config_manager.load_config() + ignored = self._SYSTEM_CONFIG_KEYS | self._secrets_top_level_keys() for plugin_id, plugin_config in config.items(): if not isinstance(plugin_config, dict): continue - if plugin_id in self._SYSTEM_CONFIG_KEYS: + if plugin_id in ignored: continue state[plugin_id] = { 'enabled': plugin_config.get('enabled', True), @@ -367,8 +419,18 @@ class StateReconciliation: """Attempt to fix an inconsistency.""" try: if inconsistency.inconsistency_type == InconsistencyType.PLUGIN_MISSING_IN_CONFIG: - # Add plugin to config with default disabled state config = self.config_manager.load_config() + if inconsistency.plugin_id in config: + # Detection said "not in config" but it is there -- the + # config changed under us, or the id came from a key merged + # in from elsewhere. Assigning the stub below would replace + # the real entry: one reported case would have traded 4.9KB + # of league settings for {'enabled': False}. Nothing to fix. + self.logger.info( + "Skipped: %s is already in config; not overwriting it", + inconsistency.plugin_id) + return True + # Add plugin to config with default disabled state config[inconsistency.plugin_id] = { 'enabled': False } diff --git a/src/startup_validator.py b/src/startup_validator.py index 79275dc2..36065bf0 100644 --- a/src/startup_validator.py +++ b/src/startup_validator.py @@ -111,17 +111,25 @@ class StartupValidator: if not template.is_file() or not installed.is_file(): continue - # The template carries placeholders the installer substitutes, - # so compare the substituted form rather than the raw file. - expected = template.read_text(encoding="utf-8") - expected = expected.replace("__PROJECT_ROOT_DIR__", str(project_root)) - expected = expected.replace("__USER__", "root") - try: actual = installed.read_text(encoding="utf-8") except PermissionError: continue + # The template carries placeholders the installer substitutes, + # so compare the substituted form rather than the raw file. + expected = template.read_text(encoding="utf-8") + expected = expected.replace("__PROJECT_ROOT_DIR__", str(project_root)) + # User= is an install-time decision, not something the template + # dictates: the installers write whoever ran them, which on a + # non-root install is never "root". Substituting a fixed "root" + # here reported drift on every such install, permanently -- and + # re-running the installer, which is what the warning tells you + # to do, could not clear it. Taking the installed unit's own + # value keeps the comparison on the directives the template + # actually controls. + expected = expected.replace("__USER__", self._installed_user(actual)) + if self._unit_body(expected) != self._unit_body(actual): self.warnings.append( f"{installed.name} differs from {template_rel}; the " @@ -132,6 +140,19 @@ class StartupValidator: except OSError as e: self.logger.debug("Could not compare systemd units: %s", e) + @staticmethod + def _installed_user(unit_text: str) -> str: + """The installed unit's ``User=``, or "root" when it does not set one. + + systemd itself defaults to root for a system unit with no User=, so that + is the right fallback rather than an empty string. + """ + for line in unit_text.splitlines(): + stripped = line.strip() + if stripped.startswith("User="): + return stripped.split("=", 1)[1].strip() + return "root" + @staticmethod def _unit_body(text: str) -> str: """A unit's meaningful lines, in order: no comments, no blanks. diff --git a/src/web_interface/api_helpers.py b/src/web_interface/api_helpers.py index 3cff5293..be2b0771 100644 --- a/src/web_interface/api_helpers.py +++ b/src/web_interface/api_helpers.py @@ -105,27 +105,3 @@ def validate_request_json(required_fields: list, data: Optional[Dict] = None) -> ) return data, None - - -def validate_request_params(required_params: list) -> Tuple[Optional[Dict], Optional[Any]]: - """ - Validate request has required query parameters. - - Args: - required_params: List of required parameter names - - Returns: - Tuple of (params_dict, error_response) or (params_dict, None) if valid - """ - missing_params = [param for param in required_params if param not in request.args] - if missing_params: - return None, error_response( - ErrorCode.INVALID_INPUT, - f"Missing required parameters: {', '.join(missing_params)}", - context={'missing_params': missing_params}, - status_code=400 - ) - - params = {param: request.args.get(param) for param in required_params} - return params, None - diff --git a/systemd/ledmatrix-web.service b/systemd/ledmatrix-web.service index 4dcb9fd7..3b74b569 100644 --- a/systemd/ledmatrix-web.service +++ b/systemd/ledmatrix-web.service @@ -1,9 +1,13 @@ -# This is a template file. The actual service file is generated dynamically -# by install_web_service.sh with the correct paths based on where the project -# is installed. Do not use this file directly - use install_web_service.sh instead. +# This is a template. Do not install it directly -- run +# scripts/install/install_web_service.sh (or install_service.sh, which +# renders this same file). Both substitute: +# __PROJECT_ROOT_DIR__ -> the directory the project is installed in +# __USER__ -> the user the web interface runs as # -# Template - paths will be replaced dynamically: -# __PROJECT_ROOT_DIR__ will be replaced with the actual project directory +# This file is the only description of the unit. It used to be duplicated as +# an inline heredoc in both installers, which drifted apart; src/startup_validator.py +# compares the installed unit against this template, so a second copy means a +# permanent, unclearable drift warning on every boot. [Unit] Description=LED Matrix Web Interface Service @@ -12,7 +16,7 @@ Wants=network-online.target [Service] Type=simple -User=root +User=__USER__ WorkingDirectory=__PROJECT_ROOT_DIR__ Environment=USE_THREADING=1 ExecStart=/usr/bin/python3 __PROJECT_ROOT_DIR__/scripts/utils/start_web_conditionally.py diff --git a/test/test_live_status_fields.py b/test/test_live_status_fields.py new file mode 100644 index 00000000..571683de --- /dev/null +++ b/test/test_live_status_fields.py @@ -0,0 +1,103 @@ +"""Tests for the metrics the live status stream reports. + +Two gaps made a struggling 1GB Pi look healthy on the dashboard: + +* ``disk_used_percent`` was the literal ``0`` in the SSE generator, so every + consumer of the live stream displayed 0% disk no matter how full the card was. + /api/v3/system/status computed it properly, but the live view -- the one people + actually watch -- did not. + +* ``memory_available_mb`` was absent. /api/v3/system/status carries it with a + comment explaining that MemAvailable, not used%, is what separates a board + that is fine from one about to fail fork(). The live stream omitted the one + number that predicts the failure. + +A wrong number is worse than a missing one, so an unreadable disk now reports +None (the UI renders '--') rather than a confident 0. + +These import web_interface.system_metrics rather than web_interface.app on +purpose: importing the app constructs a Flask application and a CacheManager, +and the latter claims the cache directory with a cleanup thread -- which broke +test_cache_cleanup_thread_ownership and the starlark route tests when this file +first reached for the generator directly. +""" + +import sys +from pathlib import Path +from types import SimpleNamespace + +import pytest + +PROJECT_ROOT = Path(__file__).parent.parent +sys.path.insert(0, str(PROJECT_ROOT)) + +from web_interface.system_metrics import collect_system_metrics # noqa: E402 + + +@pytest.fixture +def metrics(monkeypatch): + """collect_system_metrics() with psutil's readings under our control.""" + def _collect(disk_percent=12.0, available_bytes=512 * 1024 * 1024, + used_percent=67.7, raise_disk=False): + import psutil + if raise_disk: + monkeypatch.setattr(psutil, "disk_usage", + lambda _p: (_ for _ in ()).throw(OSError("no such mount"))) + else: + monkeypatch.setattr(psutil, "disk_usage", + lambda _p: SimpleNamespace(percent=disk_percent)) + monkeypatch.setattr(psutil, "virtual_memory", + lambda: SimpleNamespace(percent=used_percent, + available=available_bytes)) + return collect_system_metrics() + return _collect + + +class TestDiskUsageIsReal: + def test_reported_value_comes_from_the_filesystem(self, metrics): + # The regression: this field was the literal 0. + assert metrics(disk_percent=91.4)["disk_used_percent"] == 91.4 + + def test_a_full_disk_is_not_reported_as_zero(self, metrics): + assert metrics(disk_percent=99.9)["disk_used_percent"] != 0 + + def test_unreadable_disk_is_null_not_a_confident_zero(self, metrics): + # null renders as '--'; 0 would read as "plenty of room". + assert metrics(raise_disk=True)["disk_used_percent"] is None + + +class TestMemAvailableIsReported: + def test_mem_available_is_present_and_in_mb(self, metrics): + got = metrics(available_bytes=292 * 1024 * 1024)["memory_available_mb"] + assert got == pytest.approx(292.0, abs=0.1) + + def test_the_1gb_danger_case_is_visible(self, metrics): + """67% used can be fine or nearly out of memory; only MemAvailable says.""" + m = metrics(used_percent=67.7, available_bytes=40 * 1024 * 1024) + assert m["memory_used_percent"] == 67.7 + assert m["memory_available_mb"] == pytest.approx(40.0, abs=0.1) + + +class TestEveryPredictiveFieldIsPresent: + def test_nothing_the_dashboard_needs_is_missing(self, metrics): + m = metrics() + for field in ("cpu_percent", "cpu_temp", "memory_used_percent", + "memory_available_mb", "disk_used_percent"): + assert field in m, f"{field} missing from collected metrics" + + def test_absent_psutil_still_yields_every_key(self, monkeypatch): + """The no-psutil fallback must not drop fields the UI reads.""" + import builtins + real_import = builtins.__import__ + + def _no_psutil(name, *args, **kwargs): + if name == "psutil": + raise ImportError("psutil missing") + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", _no_psutil) + m = collect_system_metrics() + for field in ("cpu_percent", "cpu_temp", "memory_used_percent", + "memory_available_mb", "disk_used_percent"): + assert field in m + assert m["disk_used_percent"] is None diff --git a/test/test_live_update_cadence_integration.py b/test/test_live_update_cadence_integration.py new file mode 100644 index 00000000..20724b27 --- /dev/null +++ b/test/test_live_update_cadence_integration.py @@ -0,0 +1,159 @@ +"""The scheduler actually polls faster while a plugin reports live content. + +test_plugin_dynamic_update_interval.py proves _get_plugin_update_interval() +returns the plugin's requested number. That is not the same claim as "the +plugin gets updated more often", and the gap between those two is exactly where +the original bug lived: the plugin knew it wanted 15s, said so in +live_update_interval, and nothing downstream acted on it. + +So this drives the real run_scheduled_updates() over a simulated hour of ticks +and counts dispatches. It is the test that would have caught the reported bug: + + "the football plugin with live games only updates the live game in + progress if I restart the display" + +Measured on a rig before the fix, during an NFL fourth quarter, ESPN was polled +at 23:21:49 / 23:22:50 / 23:23:50 / 23:24:50 -- exactly the manifest's 60s, +never the configured 15. +""" + +from unittest.mock import MagicMock + +import pytest + +from src.plugin_system.plugin_manager import PluginManager +from src.plugin_system.plugin_state import PluginState + + +class _StateManager: + """Enough of the real state machine for reservation to behave.""" + + def __init__(self): + self.states = {} + + def can_execute(self, plugin_id): + return self.states.get(plugin_id, PluginState.ENABLED) != PluginState.RUNNING + + def set_state(self, plugin_id, state): + self.states[plugin_id] = state + + +class _Plugin: + """A sports plugin: fast while something is live, quiet otherwise.""" + + enabled = True + + def __init__(self, live_interval=15): + self.live = False + self._live_interval = live_interval + + def get_update_interval(self): + return self._live_interval if self.live else None + + def update(self): + pass + + +@pytest.fixture +def scheduler(): + pm = PluginManager.__new__(PluginManager) + import threading + pm._update_interval_cache = {} + pm.plugin_manifests = {"sports": {"update_interval": 60}} + pm.config_manager = None + pm.logger = MagicMock() + pm.state_manager = _StateManager() + pm.health_tracker = None + pm._synchronous_updates = False + pm._reservation_lock = threading.Lock() + pm._plugin_last_update_lock = threading.Lock() + pm.plugin_last_update = {} + pm.dispatched = [] + + # Stand in for the background worker: record the dispatch and complete it, + # so the next tick can reserve the plugin again. + def _enqueue(plugin_id, scheduled_time): + pm.dispatched.append(scheduled_time) + pm.plugin_last_update[plugin_id] = scheduled_time + pm.state_manager.set_state(plugin_id, PluginState.ENABLED) + + pm._enqueue_update = _enqueue + return pm + + +def _run_for(pm, plugin, seconds, start=1_000_000.0, step=1.0): + """Tick the real scheduler once a second for `seconds`.""" + pm.plugins = {"sports": plugin} + t = start + end = start + seconds + while t < end: + pm.run_scheduled_updates(current_time=t) + t += step + return len(pm.dispatched) + + +class TestTheReportedBug: + def test_a_live_game_is_polled_at_the_live_interval(self, scheduler): + """15s while live, not the manifest's 60s. This is the fix.""" + plugin = _Plugin(live_interval=15) + plugin.live = True + count = _run_for(scheduler, plugin, seconds=600) # ten minutes + # 600s / 15s = 40, allowing one for the first tick's free run. + assert 38 <= count <= 41, ( + f"{count} updates in 10 minutes of a live game; expected ~40 at 15s. " + f"At the manifest's 60s it would be ~10 -- the reported bug.") + + def test_the_same_window_without_the_hook_is_the_old_behaviour(self, scheduler): + """Pin what the bug actually looked like, so the contrast is asserted.""" + plugin = _Plugin() + plugin.live = True + plugin.get_update_interval = lambda: None # pre-fix: no opinion + count = _run_for(scheduler, plugin, seconds=600) + assert 9 <= count <= 11, ( + f"{count} updates in 10 minutes; expected ~10 at the manifest's 60s") + + def test_idle_polling_is_unchanged(self, scheduler): + """The regression that would be worse than the bug. + + Asking for 15s year-round would poll ESPN four times a minute all + summer. Idle must stay exactly on the manifest. + """ + plugin = _Plugin() + plugin.live = False + count = _run_for(scheduler, plugin, seconds=3600) # a full hour idle + assert 59 <= count <= 61, ( + f"{count} updates in an idle hour; expected ~60 at the manifest's 60s") + + +class TestTheCadenceTracksTheGame: + def test_it_speeds_up_when_a_game_starts_and_slows_when_it_ends(self, scheduler): + """No reload, no restart -- the whole point of a per-tick hook.""" + plugin = _Plugin(live_interval=15) + pm = scheduler + + _run_for(pm, plugin, seconds=300, start=1_000_000.0) + idle_before = len(pm.dispatched) + + plugin.live = True + _run_for(pm, plugin, seconds=300, start=1_000_300.0) + during = len(pm.dispatched) - idle_before + + plugin.live = False + _run_for(pm, plugin, seconds=300, start=1_000_600.0) + idle_after = len(pm.dispatched) - idle_before - during + + assert during > idle_before * 3, ( + f"live window got {during} updates vs {idle_before} idle; " + "the hook did not speed anything up") + assert idle_after <= idle_before + 1, ( + f"{idle_after} updates after the game ended vs {idle_before} before; " + "the fast cadence leaked past the live window") + + +def test_a_broken_hook_does_not_stop_the_plugin_updating(scheduler): + """A scheduler that propagates one plugin's bug stops every other plugin.""" + plugin = _Plugin() + plugin.get_update_interval = MagicMock(side_effect=RuntimeError("boom")) + count = _run_for(scheduler, plugin, seconds=600) + assert 9 <= count <= 11, ( + f"{count} updates; a raising hook should fall back to the manifest's 60s") diff --git a/test/test_logo_helper.py b/test/test_logo_helper.py index cb5824b2..0ae7ec44 100644 --- a/test/test_logo_helper.py +++ b/test/test_logo_helper.py @@ -24,7 +24,8 @@ import pytest import requests from PIL import Image, UnidentifiedImageError -from src.common.logo_helper import MAX_LOGO_BYTES, LogoHelper +from src.common.logo_helper import (MAX_LOGO_BYTES, MISSING_LOGO_RECHECK_SECONDS, + LogoHelper) @pytest.fixture(autouse=True) @@ -494,3 +495,70 @@ class TestStalePlaceholderHandling: helper.load_logo_with_download("COLL", path, "http://x/c.png") assert should_attempt_download(path) is False + + +class TestMissingLogosAreRememberedNotRewarned: + """A file that is not there does not become there by being asked again. + + load_logo() stat'd the path and logged a WARNING on every call, so a + permanently absent logo produced one warning per rotation for as long as + the process ran -- measured at 114 lines in 24 hours for a single missing + ticker icon. The positive cache never covered this because a miss returns + None and caches nothing. + """ + + def test_a_missing_logo_warns_once(self, helper, tmp_path, caplog): + absent = tmp_path / "VOO.png" + with caplog.at_level(logging.WARNING): + for _ in range(50): + assert helper.load_logo("VOO", absent) is None + assert caplog.text.count("Logo not found for VOO") == 1, ( + f"warned {caplog.text.count('Logo not found for VOO')} times in 50 calls") + + def test_the_miss_is_not_remembered_forever(self, helper, tmp_path, monkeypatch): + """A logo written later -- logo_downloader does this at runtime -- must + still be picked up without a restart.""" + path = tmp_path / "LATE.png" + assert helper.load_logo("LATE", path) is None + + write_logo(path) + # Still inside the recheck window: the remembered miss stands. + assert helper.load_logo("LATE", path) is None + + # Once it ages out, the next call stats the disk again and finds it. + for key in helper._missing_logos: + helper._missing_logos[key] -= MISSING_LOGO_RECHECK_SECONDS + 1 + assert helper.load_logo("LATE", path) is not None + + def test_a_download_clears_the_miss_immediately(self, helper, tmp_path): + """load_logo_with_download() must not be defeated by its own miss record. + + load_logo() consults _missing_logos before it stats the disk, so a + logo we just downloaded would stay invisible for the whole recheck + window without the explicit invalidation. + """ + path = tmp_path / "NEW.png" + assert helper.load_logo("NEW", path) is None + assert any(k.startswith("NEW_") for k in helper._missing_logos) + + def _fake_download(url, file_path): + write_logo(Path(file_path)) + + with patch.object(helper, "_download_logo", side_effect=_fake_download): + got = helper.load_logo_with_download("NEW", path, logo_url="http://x/NEW.png") + assert got is not None, "a freshly downloaded logo was hidden by the miss record" + assert not any(k.startswith("NEW_") for k in helper._missing_logos) + + def test_clear_cache_forgets_misses_too(self, helper, tmp_path): + helper.load_logo("GONE", tmp_path / "GONE.png") + assert helper._missing_logos + helper.clear_cache() + assert helper._missing_logos == {} + + def test_a_present_logo_is_unaffected(self, helper, tmp_path, caplog): + path = write_logo(tmp_path / "OK.png") + with caplog.at_level(logging.WARNING): + for _ in range(10): + assert helper.load_logo("OK", path) is not None + assert "Logo not found" not in caplog.text + assert helper._missing_logos == {} diff --git a/test/test_plugin_dynamic_update_interval.py b/test/test_plugin_dynamic_update_interval.py new file mode 100644 index 00000000..bd70fce0 --- /dev/null +++ b/test/test_plugin_dynamic_update_interval.py @@ -0,0 +1,153 @@ +"""A plugin can ask to be polled faster while it has something live. + +The manifest carries one static `update_interval`, and until now that was the +only thing the scheduler would look at. A sports scoreboard needs 15 seconds +while a game is in progress and 15 minutes when nothing is on, and no single +number expresses that. + +Measured consequence, on a live rig during an NFL game in its fourth quarter: +football-scoreboard's manifest pins update_interval to 60, so ESPN was polled +exactly once a minute -- + + 23:21:49 23:22:50 23:23:50 23:24:50 23:25:50 + +-- while the plugin's own live_update_interval said 15. The clock and score on +the panel therefore lagged by up to a minute during a two-minute drill, which +reads to a viewer as a frozen display. + +get_update_interval() lets the plugin say what it needs per tick. These tests +pin the parts that are easy to get wrong: that the hook wins, that "no opinion" +falls back cleanly, that a broken hook cannot stop a plugin updating, and that +the hook is not accidentally cached (which would defeat the whole point). +""" + +from unittest.mock import MagicMock + +import pytest + +from src.plugin_system.plugin_manager import PluginManager + + +@pytest.fixture +def manager(): + pm = PluginManager.__new__(PluginManager) + pm._update_interval_cache = {} + pm.plugin_manifests = {"sports": {"update_interval": 60}} + pm.config_manager = None + pm.logger = MagicMock() + return pm + + +class _Plugin: + def __init__(self, wants=None, raises=False): + self._wants = wants + self._raises = raises + self.calls = 0 + + def get_update_interval(self): + self.calls += 1 + if self._raises: + raise RuntimeError("plugin is broken") + return self._wants + + +def test_the_hook_overrides_the_manifest(manager): + """The whole point: 15s while live, not the manifest's 60.""" + assert manager._get_plugin_update_interval("sports", _Plugin(wants=15)) == 15.0 + + +def test_none_means_no_opinion_and_the_manifest_applies(manager): + """A plugin with nothing live should not have to restate the default.""" + assert manager._get_plugin_update_interval("sports", _Plugin(wants=None)) == 60.0 + + +def test_a_plugin_without_the_hook_is_unaffected(manager): + """Every existing plugin predates this and must keep its manifest value.""" + assert manager._get_plugin_update_interval("sports", MagicMock(spec=[])) == 60.0 + + +def test_the_hook_is_consulted_every_time(manager): + """Caching the answer would make it exactly as static as the manifest. + + The static path *is* cached -- reading config is expensive -- so it would be + an easy mistake to let the dynamic answer ride along in that cache. + """ + plugin = _Plugin(wants=15) + for _ in range(5): + manager._get_plugin_update_interval("sports", plugin) + assert plugin.calls == 5, f"hook called {plugin.calls} times in 5 lookups" + + +def test_the_cadence_can_change_between_ticks(manager): + """A game going final must slow the polling back down without a reload.""" + plugin = _Plugin(wants=15) + assert manager._get_plugin_update_interval("sports", plugin) == 15.0 + plugin._wants = None # game ended + assert manager._get_plugin_update_interval("sports", plugin) == 60.0 + plugin._wants = 15 # another game started + assert manager._get_plugin_update_interval("sports", plugin) == 15.0 + + +class TestABrokenHookCannotStopUpdates: + """A scheduler that propagates a plugin's bug stops every other plugin too.""" + + def test_a_raising_hook_falls_back_to_the_manifest(self, manager): + assert manager._get_plugin_update_interval("sports", _Plugin(raises=True)) == 60.0 + + @pytest.mark.parametrize("junk", ["soon", object(), [], {}]) + def test_a_non_numeric_hook_falls_back(self, manager, junk): + assert manager._get_plugin_update_interval("sports", _Plugin(wants=junk)) == 60.0 + + @pytest.mark.parametrize("junk", [float("nan"), float("inf"), float("-inf")]) + def test_nan_and_infinity_fall_back(self, manager, junk): + assert manager._get_plugin_update_interval("sports", _Plugin(wants=junk)) == 60.0 + + @pytest.mark.parametrize("junk", [True, False]) + def test_a_bool_hook_falls_back(self, manager, junk): + """bool is a subclass of int, so it must be rejected before float() + turns it into 0.0/1.0 and the floor silently swallows the mistake.""" + assert manager._get_plugin_update_interval("sports", _Plugin(wants=junk)) == 60.0 + + +class TestTheFloor: + """A plugin asking for 0 would be re-entered on every tick of the render + loop, which is a busy-wait against whatever API it fetches.""" + + @pytest.mark.parametrize("wants", [0, 0.5, -10]) + def test_small_and_negative_requests_are_clamped(self, manager, wants): + got = manager._get_plugin_update_interval("sports", _Plugin(wants=wants)) + assert got == PluginManager.MIN_DYNAMIC_UPDATE_INTERVAL + + def test_a_reasonable_request_is_not_clamped(self, manager): + assert manager._get_plugin_update_interval("sports", _Plugin(wants=15)) == 15.0 + + +def test_the_static_path_still_prefers_the_manifest_over_config(): + """Deliberately unchanged, and not an oversight. + + Config values that have sat inert behind the manifest are stale by + definition -- nothing has been honouring them. On one rig, football and + baseball both carry update_interval 3600 in config against a manifest 60; + making config win would slow those plugins by 60x. The dynamic hook is the + supported way for a plugin to vary its own cadence, so flipping this is + both risky and no longer necessary. + """ + pm = PluginManager.__new__(PluginManager) + pm._update_interval_cache = {} + pm.plugin_manifests = {"sports": {"update_interval": 60}} + cfg = MagicMock() + cfg.get_config.return_value = {"sports": {"update_interval": 3600}} + pm.config_manager = cfg + pm.logger = MagicMock() + assert pm._get_plugin_update_interval("sports", MagicMock(spec=[])) == 60.0 + + +def test_config_is_still_used_when_the_manifest_is_silent(): + pm = PluginManager.__new__(PluginManager) + pm._update_interval_cache = {} + pm.plugin_manifests = {"sports": {}} + cfg = MagicMock() + cfg.get_config.return_value = {"sports": {"update_interval": 120}} + pm.config_manager = cfg + pm.logger = MagicMock() + assert pm._get_plugin_update_interval("sports", MagicMock(spec=[])) == 120.0 diff --git a/test/test_reconciliation_phantom_plugins.py b/test/test_reconciliation_phantom_plugins.py new file mode 100644 index 00000000..d05e80fa --- /dev/null +++ b/test/test_reconciliation_phantom_plugins.py @@ -0,0 +1,170 @@ +"""Tests for plugin-state reconciliation reporting phantom inconsistencies. + +Observed on a device running four installed, configured, working plugins: the +web UI showed "Stale plugin config entries found: football-scoreboard, +odds-ticker, data, ledmatrix-weather, starlark-apps. Remove them from +config.json" for hours. Three separate defects combined to produce that: + +1. load_config() merges config_secrets.json into the config it returns, and the + ignore list named only 'github' and 'youtube'. A 'data' key in the secrets + file therefore became a phantom plugin, reported as in-config-not-on-disk. + +2. The auto-fix for "on disk but not in config" assigned + ``config[plugin_id] = {'enabled': False}`` unconditionally. When detection was + wrong, that traded a plugin's real configuration (4.9KB of league settings in + the reported case) for a stub. It only avoided firing there because the write + failed with EACCES. + +3. The verdict is a snapshot written once per run and served to the UI from a + status file, and a run that fails to apply a fix also declines to retry. So a + condition that had since resolved was reported indefinitely. +""" + +import json +from pathlib import Path +from unittest.mock import Mock + +import pytest + +from src.plugin_system.state_reconciliation import ( + InconsistencyType, + StateReconciliation, + still_unresolved, +) + + +class _ConfigManager: + """Minimal config manager with the secrets path the real one exposes.""" + + def __init__(self, config, secrets_path=None): + self._config = config + self._secrets_path = secrets_path + self.saved = [] + + def load_config(self): + return dict(self._config) + + def save_config(self, config): + self.saved.append(config) + self._config = dict(config) + + def get_secrets_path(self): + return self._secrets_path + + +def _reconciler(config, plugins_dir, secrets_path=None): + return StateReconciliation( + state_manager=Mock(), + config_manager=_ConfigManager(config, secrets_path), + plugin_manager=Mock(), + plugins_dir=plugins_dir, + ) + + +class TestSecretsKeysAreNotPlugins: + def test_secrets_key_is_not_treated_as_a_plugin(self, tmp_path): + secrets = tmp_path / "config_secrets.json" + # The shape actually found on the device: display state written here. + secrets.write_text(json.dumps({"data": {"mode": "nfl_recent"}, + "timestamp": 1.0}), encoding="utf-8") + config = { + "data": {"mode": "nfl_recent"}, + "football-scoreboard": {"enabled": True, "nfl": {"favorite_teams": ["TB"]}}, + } + state = _reconciler(config, tmp_path, secrets).\ + _get_config_state() + + assert "data" not in state, "secrets key leaked in as a phantom plugin" + assert "football-scoreboard" in state + + def test_missing_secrets_file_is_survivable(self, tmp_path): + state = _reconciler({"odds-ticker": {"enabled": True}}, tmp_path, + tmp_path / "does-not-exist.json")._get_config_state() + assert "odds-ticker" in state + + def test_non_path_secrets_location_is_survivable(self, tmp_path): + """Regression: a non-path return raised TypeError out of the helper, + which the broad handler in _get_config_state() swallowed as "Error + reading config state" -- emptying the config state and making every + downstream detection wrong.""" + r = _reconciler({"odds-ticker": {"enabled": True}}, tmp_path, + secrets_path=Mock()) + assert "odds-ticker" in r._get_config_state() + + def test_config_manager_without_secrets_path_is_survivable(self, tmp_path): + r = StateReconciliation(state_manager=Mock(), + config_manager=Mock(spec=["load_config", "save_config"]), + plugin_manager=Mock(), plugins_dir=tmp_path) + r.config_manager.load_config.return_value = {"odds-ticker": {"enabled": True}} + assert "odds-ticker" in r._get_config_state() + + +class TestFixNeverClobbersRealConfig: + def _missing_in_config(self, plugin_id): + inc = Mock() + inc.inconsistency_type = InconsistencyType.PLUGIN_MISSING_IN_CONFIG + inc.plugin_id = plugin_id + return inc + + def test_existing_entry_is_not_overwritten(self, tmp_path): + rich = {"enabled": True, "nfl": {"favorite_teams": ["TB"], "show_odds": True}} + r = _reconciler({"football-scoreboard": dict(rich)}, tmp_path) + + assert r._fix_inconsistency(self._missing_in_config("football-scoreboard")) is True + # The regression: this wrote {'enabled': False} over the real settings. + assert r.config_manager.saved == [] + assert r.config_manager.load_config()["football-scoreboard"] == rich + + def test_genuinely_absent_plugin_is_still_added(self, tmp_path): + r = _reconciler({"other-plugin": {"enabled": True}}, tmp_path) + + assert r._fix_inconsistency(self._missing_in_config("new-plugin")) is True + assert len(r.config_manager.saved) == 1 + assert r.config_manager.load_config()["new-plugin"] == {"enabled": False} + # and it must not disturb what was already there + assert r.config_manager.load_config()["other-plugin"] == {"enabled": True} + + +class TestStaleFindingsAreDropped: + IN_CONFIG = InconsistencyType.PLUGIN_MISSING_IN_CONFIG.value + ON_DISK = InconsistencyType.PLUGIN_MISSING_ON_DISK.value + + def test_resolved_missing_in_config_is_dropped(self): + entries = [{"plugin_id": "football-scoreboard", "type": self.IN_CONFIG}] + assert still_unresolved(entries, {"football-scoreboard"}, set()) == [] + + def test_genuinely_missing_in_config_is_kept(self): + entries = [{"plugin_id": "football-scoreboard", "type": self.IN_CONFIG}] + assert still_unresolved(entries, set(), set()) == entries + + def test_resolved_missing_on_disk_is_dropped(self): + entries = [{"plugin_id": "odds-ticker", "type": self.ON_DISK}] + assert still_unresolved(entries, set(), {"odds-ticker"}) == [] + + def test_genuinely_missing_on_disk_is_kept(self): + entries = [{"plugin_id": "data", "type": self.ON_DISK}] + assert still_unresolved(entries, set(), {"odds-ticker"}) == entries + + def test_unrecheckable_kinds_are_kept(self): + # Filtering must only ever remove what it can prove stale. + entries = [{"plugin_id": "x", "type": "plugin_version_mismatch"}] + assert still_unresolved(entries, set(), set()) == entries + + def test_the_reported_device_state_clears_every_false_finding(self): + """The exact verdict the device served, against its real state.""" + entries = [ + {"plugin_id": "football-scoreboard", "type": self.IN_CONFIG}, + {"plugin_id": "odds-ticker", "type": self.IN_CONFIG}, + {"plugin_id": "data", "type": self.ON_DISK}, + {"plugin_id": "ledmatrix-weather", "type": self.IN_CONFIG}, + {"plugin_id": "starlark-apps", "type": self.IN_CONFIG}, + ] + installed = {"football-scoreboard", "ledmatrix-weather", "odds-ticker", + "starlark-apps", "web-ui-info"} + live = still_unresolved(entries, installed, installed) + + # All four "installed but not in config" findings were false and clear. + # 'data' legitimately is not installed, so this filter keeps it; it stops + # being reported because _get_config_state() no longer invents it from the + # secrets file, which takes effect on the next reconciliation run. + assert [e["plugin_id"] for e in live] == ["data"] diff --git a/test/test_reconciliation_status_endpoint.py b/test/test_reconciliation_status_endpoint.py new file mode 100644 index 00000000..78a744aa --- /dev/null +++ b/test/test_reconciliation_status_endpoint.py @@ -0,0 +1,112 @@ +"""The reconciliation-status endpoint must re-check its stored verdict. + +#557 added _drop_stale_reconciliation_findings() so a resolved condition stops +being reported: the verdict is a snapshot written once per run, and a run that +fails to apply a fix also declines to retry, so a device whose plugins were all +present in config kept being told for hours that four of them were missing. + +That wiring had no test. Only the pure still_unresolved() helper in +src/plugin_system/ was covered, which lives outside web_interface and therefore +survived the api_v3 blueprint split untouched -- so when #553 moved this +endpoint into api_v3/plugins.py, losing the filter would have been completely +silent. This test exists so that cannot happen again. +""" + +import json +import sys +import tempfile +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +from flask import Flask + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from web_interface.blueprints.api_v3 import api_v3 # noqa: E402 + +IN_CONFIG = "plugin_missing_in_config" +ON_DISK = "plugin_missing_on_disk" + + +@pytest.fixture +def client(tmp_path, monkeypatch): + """App whose status file, config and plugins dir are all under our control.""" + monkeypatch.setattr(tempfile, "gettempdir", lambda: str(tmp_path)) + + plugins_dir = tmp_path / "plugin-repos" + plugins_dir.mkdir() + + def _install(plugin_id): + d = plugins_dir / plugin_id + d.mkdir() + (d / "manifest.json").write_text(json.dumps({"id": plugin_id}), encoding="utf-8") + + def _setup(verdict, config=None, installed=()): + (tmp_path / "ledmatrix_reconciliation.json").write_text( + json.dumps(verdict), encoding="utf-8") + for pid in installed: + _install(pid) + api_v3.config_manager = MagicMock() + api_v3.config_manager.load_config.return_value = dict(config or {}) + api_v3.plugin_manager = MagicMock() + api_v3.plugin_manager.plugins_dir = str(plugins_dir) + + app = Flask(__name__) + app.config["TESTING"] = True + app.register_blueprint(api_v3, url_prefix="/api/v3") + return app.test_client() + + return _setup + + +def _get(c): + return c.get("/api/v3/plugins/reconciliation-status").get_json()["data"] + + +class TestStaleFindingsAreDroppedByTheEndpoint: + def test_a_plugin_now_in_config_is_no_longer_reported(self, client): + c = client({"done": True, "unresolved": [ + {"plugin_id": "football-scoreboard", "type": IN_CONFIG}]}, + config={"football-scoreboard": {"enabled": True}}) + # The regression: this kept being reported for hours after it resolved. + assert _get(c)["unresolved"] == [] + + def test_a_plugin_now_installed_is_no_longer_reported(self, client): + c = client({"done": True, "unresolved": [ + {"plugin_id": "odds-ticker", "type": ON_DISK}]}, + installed=["odds-ticker"]) + assert _get(c)["unresolved"] == [] + + def test_a_finding_that_still_holds_is_kept(self, client): + c = client({"done": True, "unresolved": [ + {"plugin_id": "ghost-plugin", "type": ON_DISK}]}) + assert [e["plugin_id"] for e in _get(c)["unresolved"]] == ["ghost-plugin"] + + def test_the_reported_device_verdict_clears(self, client): + """The five findings the live device served, against its real state.""" + installed = ["football-scoreboard", "ledmatrix-weather", + "odds-ticker", "starlark-apps"] + c = client({"done": True, "unresolved": [ + {"plugin_id": p, "type": IN_CONFIG} for p in installed]}, + config={p: {"enabled": True} for p in installed}, + installed=installed) + assert _get(c)["unresolved"] == [] + + +class TestTheEndpointStaysRobust: + def test_an_empty_verdict_is_passed_through(self, client): + c = client({"done": True, "unresolved": []}) + assert _get(c) == {"done": True, "unresolved": []} + + def test_a_broken_config_manager_leaves_findings_untouched(self, client): + """Best-effort: a stale warning beats a failed endpoint.""" + c = client({"done": True, "unresolved": [ + {"plugin_id": "football-scoreboard", "type": IN_CONFIG}]}) + api_v3.config_manager.load_config.side_effect = OSError("config unreadable") + body = _get(c) + assert [e["plugin_id"] for e in body["unresolved"]] == ["football-scoreboard"] + + def test_a_run_still_in_progress_is_reported_as_such(self, client): + c = client({"done": False, "unresolved": []}) + assert _get(c)["done"] is False diff --git a/test/test_sports_logo_cache_bounded.py b/test/test_sports_logo_cache_bounded.py new file mode 100644 index 00000000..8570b81e --- /dev/null +++ b/test/test_sports_logo_cache_bounded.py @@ -0,0 +1,120 @@ +"""Tests that SportsCore's decoded-logo cache is LRU-bounded. + +``self._logo_cache`` was a plain dict keyed by team abbreviation, with no +eviction. The entries are decoded RGBA thumbnails sized to display*1.5 -- about +36KB on a 256x64 panel, more for wide wordmarks -- and +assets/sports/ncaa_logos ships 307 of them. A plugin that walked a full league +therefore held the whole league in memory: roughly 11-18MB per manager +instance, and a league runs three of them (live/recent/upcoming) each with its +own cache. On a 1GB Pi 3B+ with ~290MB available that is worth recovering. + +The bound has to be LRU rather than "clear when full": the logos on screen +right now are exactly the ones that must not be thrown away. +""" + +import logging +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +from PIL import Image + +# src.base_classes.sports transitively imports the hardware matrix driver. +sys.modules.setdefault("rgbmatrix", MagicMock()) + +from src.base_classes.sports import SportsCore # noqa: E402 + +LOGGER = logging.getLogger("test_sports_logo_cache_bounded") + + +class _StubSports(SportsCore): + def _fetch_data(self): + return None + + def _extract_game_details(self, game_event): + return None + + +@pytest.fixture +def host(monkeypatch, tmp_path): + monkeypatch.setattr( + SportsCore, "_initialize_logo_dir", lambda self, configured: tmp_path) + monkeypatch.setattr( + "src.base_classes.sports.core.get_background_service", + lambda *args, **kwargs: MagicMock()) + # Never reach for the network: every logo these tests ask for exists. + monkeypatch.setattr( + "src.base_classes.sports.core.download_missing_logo", + lambda *a, **k: None) + + display_manager = MagicMock() + display_manager.matrix.width = 128 + display_manager.matrix.height = 32 + display_manager.width = 128 + display_manager.height = 32 + display_manager.image = Image.new("RGB", (128, 32)) + cache_manager = MagicMock() + cache_manager.cache_dir = str(tmp_path) + instance = _StubSports({"timezone": "UTC"}, display_manager, + cache_manager, LOGGER, "nhl") + instance._logo_dir = tmp_path + return instance, tmp_path + + +def _load(host, abbrev): + """Create a real logo file for `abbrev` and load it through the cache.""" + instance, tmp_path = host + path = tmp_path / f"{abbrev}.png" + if not path.exists(): + Image.new("RGBA", (64, 64), (1, 2, 3, 255)).save(path) + return instance._load_and_resize_logo(abbrev, abbrev, path, None) + + +class TestTheCacheIsBounded: + def test_it_never_exceeds_the_limit(self, host): + instance, _ = host + limit = instance._LOGO_CACHE_MAX + for i in range(limit + 40): + assert _load(host, f"T{i}") is not None + # The regression: this grew to limit + 40, and for NCAA to 307. + assert len(instance._logo_cache) == limit + + def test_the_limit_is_smaller_than_a_real_league(self, host): + """ncaa_logos ships 307 files; the cap has to be well under that.""" + instance, _ = host + assert instance._LOGO_CACHE_MAX < 307 + + +class TestEvictionIsLRUNotArbitrary: + def test_the_least_recently_used_goes_first(self, host): + instance, _ = host + limit = instance._LOGO_CACHE_MAX + for i in range(limit): + _load(host, f"T{i}") + _load(host, "NEW") + assert "T0" not in instance._logo_cache, "oldest should have been evicted" + assert "NEW" in instance._logo_cache + + def test_touching_an_entry_saves_it(self, host): + instance, _ = host + limit = instance._LOGO_CACHE_MAX + for i in range(limit): + _load(host, f"T{i}") + _load(host, "T0") # a cache hit -- T0 is on screen again + _load(host, "NEW") # forces one eviction + + assert "T0" in instance._logo_cache, "a logo in use was thrown away" + assert "T1" not in instance._logo_cache, "T1 was the true LRU entry" + + +class TestHitsStillAvoidDiskWork: + def test_a_second_request_returns_the_cached_object(self, host, monkeypatch): + instance, _ = host + first = _load(host, "TB") + + def _boom(*a, **k): + raise AssertionError("cache hit re-opened the file from disk") + + monkeypatch.setattr(Image, "open", _boom) + assert _load(host, "TB") is first diff --git a/test/test_system_action_sudo_diagnosis.py b/test/test_system_action_sudo_diagnosis.py new file mode 100644 index 00000000..88adca02 --- /dev/null +++ b/test/test_system_action_sudo_diagnosis.py @@ -0,0 +1,131 @@ +"""A privileged system action that cannot run must say why. + +The web interface runs unprivileged. Its systemctl/reboot/journalctl calls only +work once scripts/install/configure_web_sudo.sh has granted NOPASSWD, and +first_time_install.sh never invokes that script -- so on a fresh device every +one of those actions fails. What the user got back was: + + {"message": "Action failed; see logs for details", "status": "error"} + +...and the log viewer was broken for exactly the same reason, so "see logs" led +nowhere. This is the closed loop that src/web_interface/error_handler.py's +describe_exception() was written to break; /system/action's exception handler +was simply still discarding the cause. + +Observed live: POSTing reboot_system to a Pi returned that bare sentence, and +pinning down "no passwordless sudo" took reading the installer instead of +reading the response. +""" + +import subprocess +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest +from flask import Flask + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from web_interface.blueprints.api_v3 import api_v3 # noqa: E402 + + +@pytest.fixture +def client(): + app = Flask(__name__) + app.config['TESTING'] = True + app.register_blueprint(api_v3, url_prefix='/api/v3') + return app.test_client() + + +def _act(client, action='stop_display'): + return client.post('/api/v3/system/action', json={'action': action}) + + +def _result(args, rc=0, stderr=''): + return subprocess.CompletedProcess(args, rc, stdout='', stderr=stderr) + + +class TestSudoRefusalIsNamed: + @pytest.mark.parametrize("stderr", [ + "sudo: a password is required", + "sudo: no tty present and no askpass program specified", + "sudo: a terminal is required to read the password", + ]) + def test_every_sudo_refusal_wording_is_recognised(self, client, stderr): + with patch('subprocess.run', side_effect=lambda a, **k: _result(a, 1, stderr)): + body = _act(client).get_json() + + assert body['status'] == 'error' + # The point: the response tells the user what to do about it. + assert 'configure_web_sudo.sh' in body['message'] + + def test_the_raw_stderr_is_still_returned(self, client): + with patch('subprocess.run', + side_effect=lambda a, **k: _result(a, 1, "sudo: a password is required")): + body = _act(client).get_json() + assert 'a password is required' in body['stderr'] + + +class TestUnrelatedFailuresAreNotMisattributed: + def test_a_real_systemctl_error_keeps_the_generic_message(self, client): + with patch('subprocess.run', + side_effect=lambda a, **k: _result(a, 5, "Unit ledmatrix.service not found.")): + body = _act(client).get_json() + + assert 'configure_web_sudo.sh' not in body['message'], \ + "a missing unit was blamed on sudo" + assert 'Unit ledmatrix.service not found.' in body['stderr'] + assert body['returncode'] == 5 + + +class TestTheExceptionPathNamesTheCause: + def test_the_exception_type_and_message_are_returned(self, client): + with patch('subprocess.run', side_effect=FileNotFoundError(2, "sudo")): + body = _act(client).get_json() + + # The regression: 'details' was absent and the cause was discarded. + assert 'FileNotFoundError' in body['details'] + + def test_a_sudo_shaped_exception_also_gets_the_hint(self, client): + with patch('subprocess.run', + side_effect=PermissionError("sudo: a password is required")): + body = _act(client).get_json() + assert 'configure_web_sudo.sh' in body['message'] + + +class TestTheEarlyReturnPathsAlsoGetTheHint: + """start_display with a mode builds its own response and returns before the + shared nonzero-result path, so it needed the hint separately. Found by + CodeRabbit on #560; verified against the code before fixing.""" + + def test_on_demand_start_reports_the_sudo_cause(self, client): + with patch('subprocess.run', + side_effect=lambda a, **k: _result(a, 1, "sudo: a password is required")): + body = client.post('/api/v3/system/action', + json={'action': 'start_display', 'mode': 'nfl_live'}).get_json() + + assert 'configure_web_sudo.sh' in body['message'], "the on-demand branch still reported only 'Failed to start display'" + assert body['status'] == 'error' + + def test_on_demand_start_keeps_its_success_message(self, client): + with patch('subprocess.run', side_effect=lambda a, **k: _result(a, 0)): + body = client.post('/api/v3/system/action', + json={'action': 'start_display', 'mode': 'nfl_live'}).get_json() + assert body['status'] == 'success' + assert body['message'] == 'Display started' + + def test_an_unrelated_on_demand_failure_is_not_blamed_on_sudo(self, client): + with patch('subprocess.run', + side_effect=lambda a, **k: _result(a, 5, "Unit not found.")): + body = client.post('/api/v3/system/action', + json={'action': 'start_display', 'mode': 'nfl_live'}).get_json() + assert body['message'] == 'Failed to start display' + + +class TestSuccessIsUnchanged: + def test_a_working_action_still_reports_success(self, client): + with patch('subprocess.run', side_effect=lambda a, **k: _result(a, 0)): + body = _act(client).get_json() + assert body['status'] == 'success' + assert 'configure_web_sudo.sh' not in body.get('message', '') diff --git a/test/test_systemd_unit_drift.py b/test/test_systemd_unit_drift.py index 21cf56e0..8afaadb0 100644 --- a/test/test_systemd_unit_drift.py +++ b/test/test_systemd_unit_drift.py @@ -17,6 +17,8 @@ editing files under /etc and restarting services is the installer's job, not something a display process should do to a machine while it boots. """ import logging +import shlex +import subprocess from pathlib import Path from unittest.mock import MagicMock @@ -150,3 +152,122 @@ def test_a_missing_installed_unit_is_silent(validator, tmp_path): validator._validate_systemd_units() assert not validator.warnings assert not validator.errors + + +# --- the web unit: one template, three copies, and a warning that never cleared --- +# +# install_service.sh and install_web_service.sh each carried their own inline +# heredoc of ledmatrix-web.service. install_service.sh's had drifted -- no +# Wants=network-online.target, RestartSec, SyslogIdentifier or CacheDirectory -- +# and that is what was installed on real rigs. The validator correctly reported +# the drift and told the user to re-run install_service.sh, which reinstalled the +# same stale copy, so the warning could never clear. Separately, the template +# hardcoded User=root while the installers write whoever ran them, so even the +# *correct* installer produced a permanent warning on any non-root install. + + +def _render(template_text, project_root, user): + """Exactly what the installers' sed does.""" + return (template_text + .replace("__PROJECT_ROOT_DIR__", str(project_root)) + .replace("__USER__", user)) + + +def test_the_web_unit_installed_as_a_non_root_user_is_not_drift(validator, tmp_path): + """The web interface runs as whoever installed it, not as root. + + This is the case that warned forever: nothing the user could do would make + an installed `User=pi` match a template that said `User=root`. + """ + project_root = Path("src/startup_validator.py").resolve().parent.parent + template_rel = "systemd/ledmatrix-web.service" + template = project_root / template_rel + if not template.is_file(): + pytest.skip("repo unit template not present") + + installed = tmp_path / "ledmatrix-web.service" + installed.write_text( + _render(template.read_text(encoding="utf-8"), project_root, "hdpi"), + encoding="utf-8") + + validator._UNITS = ((template_rel, str(installed)),) + validator._validate_systemd_units() + assert not validator.warnings, ( + f"a correctly installed non-root web unit warned: {validator.warnings}") + + +def test_the_web_unit_still_reports_a_real_changed_directive(validator, tmp_path): + """Ignoring User= must not make the check blind to everything else.""" + project_root = Path("src/startup_validator.py").resolve().parent.parent + template_rel = "systemd/ledmatrix-web.service" + template = project_root / template_rel + if not template.is_file(): + pytest.skip("repo unit template not present") + + rendered = _render(template.read_text(encoding="utf-8"), project_root, "hdpi") + # Drop RestartSec -- one of the directives the stale heredoc was missing. + stale = "\n".join(line for line in rendered.splitlines() if not line.startswith("RestartSec=")) + installed = tmp_path / "ledmatrix-web.service" + installed.write_text(stale + "\n", encoding="utf-8") + + validator._UNITS = ((template_rel, str(installed)),) + validator._validate_systemd_units() + assert validator.warnings, "a web unit missing RestartSec= produced no warning" + assert not validator.errors + + +def test_installed_user_falls_back_to_root(): + """systemd defaults a system unit with no User= to root, so we must too.""" + assert StartupValidator._installed_user("[Service]\nExecStart=/x\n") == "root" + assert StartupValidator._installed_user("[Service]\nUser=pi\n") == "pi" + assert StartupValidator._installed_user("[Service]\n User=hdpi \n") == "hdpi" + + +def test_sed_escape_replacement_preserves_special_characters(): + """A project path or username containing sed-special characters must render literally. + + The installers build their sed expression by interpolating a shell + variable into the replacement side of `sed s|pattern|replacement|`. + Unescaped, sed treats `&` as "insert the whole match" and `\\` as an + escape character, so a path like `/opt/led&matrix` would corrupt the + rendered unit instead of being substituted as-is. lib_systemd_render.sh's + sed_escape_replacement exists to prevent exactly that. + """ + project_root = Path("src/startup_validator.py").resolve().parent.parent + helper = project_root / "scripts" / "install" / "lib_systemd_render.sh" + if not helper.is_file(): + pytest.skip("install helper not present") + + value = "/opt/led&matrix\\pi|two" + escape_cmd = f'source {shlex.quote(str(helper))}; sed_escape_replacement {shlex.quote(value)}' + escaped = subprocess.run( + ["bash", "-c", escape_cmd], capture_output=True, text=True, check=True + ).stdout + + rendered = subprocess.run( + ["sed", f"s|__X__|{escaped}|g"], + input="path=__X__\n", capture_output=True, text=True, check=True, + ).stdout + + assert rendered == f"path={value}\n", ( + "a sed-special character in the replacement was not preserved literally") + + +def test_no_installer_carries_its_own_copy_of_a_unit(): + """The regression guard. + + Both installers used to inline the unit as a heredoc, and the two copies + drifted from the template and from each other. A unit body in a shell script + is the bug, so assert there isn't one rather than asserting the current + contents match -- matching contents is exactly what silently stops being + true. + """ + project_root = Path("src/startup_validator.py").resolve().parent.parent + offenders = [] + for script in sorted((project_root / "scripts" / "install").glob("*.sh")): + text = script.read_text(encoding="utf-8", errors="replace") + if "[Unit]" in text and "Description=" in text: + offenders.append(script.name) + assert not offenders, ( + f"{offenders} contain an inline systemd unit; render " + f"systemd/*.service instead so there is one source of truth") diff --git a/test/test_web_autostart_default.py b/test/test_web_autostart_default.py new file mode 100644 index 00000000..9119b7ed --- /dev/null +++ b/test/test_web_autostart_default.py @@ -0,0 +1,63 @@ +"""Tests for the web interface's autostart default. + +Regression under test: ``start_web_conditionally.py`` read the flag with +``config_data.get("web_display_autostart", False)``, so a config that simply +lacked the key got no web interface. Both config.template.json and +first_time_install.sh ship the key as ``true``, so absence means an older or +hand-edited config -- not a request to stay down. + +The failure was silent in the worst way: the "not starting" path exits 0, so +``systemctl status ledmatrix-web`` reported the unit as successfully started +while nothing was listening on the port, and the journal's only trace was one +line saying the flag was "false or not set". +""" + +import importlib.util +from pathlib import Path + +import pytest + +SCRIPT = (Path(__file__).resolve().parents[1] + / "scripts" / "utils" / "start_web_conditionally.py") + + +@pytest.fixture(scope="module") +def mod(): + spec = importlib.util.spec_from_file_location("start_web_conditionally", SCRIPT) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +class TestAutostartDefault: + def test_absent_key_still_starts(self, mod): + # The regression: this returned False and the UI silently stayed down. + assert mod.autostart_enabled({}) is True + + def test_unreadable_config_still_starts(self, mod): + # main() falls back to {} when the config is missing or corrupt, because + # the web interface is how a broken config gets repaired. + assert mod.autostart_enabled({}) is True + + def test_explicit_true_starts(self, mod): + assert mod.autostart_enabled({"web_display_autostart": True}) is True + + def test_explicit_false_does_not_start(self, mod): + assert mod.autostart_enabled({"web_display_autostart": False}) is False + + @pytest.mark.parametrize("value", ["off", "false", "no", "0", "OFF", " False "]) + def test_disabling_strings_do_not_start(self, mod, value): + assert mod.autostart_enabled({"web_display_autostart": value}) is False + + @pytest.mark.parametrize("value", ["on", "true", "yes", "1", "TRUE"]) + def test_enabling_strings_start(self, mod, value): + assert mod.autostart_enabled({"web_display_autostart": value}) is True + + +class TestShippedDefaultsAgree: + def test_template_ships_autostart_true(self): + """The code default must match what the installer actually writes.""" + import json + template = json.loads( + (SCRIPT.parents[2] / "config" / "config.template.json").read_text(encoding="utf-8")) + assert template["web_display_autostart"] is True diff --git a/web_interface/app.py b/web_interface/app.py index 98bf0f71..13529a1d 100644 --- a/web_interface/app.py +++ b/web_interface/app.py @@ -32,6 +32,8 @@ _JOURNALCTL = shutil.which('journalctl') _SYSTEMCTL = shutil.which('systemctl') _VCGENCMD = shutil.which('vcgencmd') +from web_interface.system_metrics import collect_system_metrics + # Create Flask app app = Flask(__name__) app.secret_key = os.urandom(24) @@ -606,26 +608,12 @@ def system_status_generator(): """Generate system status updates""" while True: try: - # Try to import psutil for system stats - try: - import psutil - # interval=None is non-blocking; primed at module startup above - cpu_percent = round(psutil.cpu_percent(interval=None), 1) - memory = psutil.virtual_memory() - memory_used_percent = round(memory.percent, 1) - - # Try to get CPU temperature (Raspberry Pi specific) - cpu_temp = 0 - try: - with open('/sys/class/thermal/thermal_zone0/temp', 'r') as f: - cpu_temp = round(float(f.read()) / 1000.0, 1) - except (OSError, ValueError): - pass - - except ImportError: - cpu_percent = 0 - memory_used_percent = 0 - cpu_temp = 0 + metrics = collect_system_metrics() + cpu_percent = metrics['cpu_percent'] + memory_used_percent = metrics['memory_used_percent'] + memory_available_mb = metrics['memory_available_mb'] + disk_used_percent = metrics['disk_used_percent'] + cpu_temp = metrics['cpu_temp'] # Check if display service is running (cached to avoid per-client subprocess forks) now = time.time() @@ -646,8 +634,9 @@ def system_status_generator(): 'service_active': service_active, 'cpu_percent': cpu_percent, 'memory_used_percent': memory_used_percent, + 'memory_available_mb': memory_available_mb, 'cpu_temp': cpu_temp, - 'disk_used_percent': 0, + 'disk_used_percent': disk_used_percent, 'power': _get_power_status() } yield status diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index 050412f7..730ecf2e 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -17,7 +17,6 @@ everything they need has to exist first. """ from flask import Blueprint, request, jsonify, Response import contextlib -import fcntl import json import os import re @@ -340,19 +339,6 @@ def _validate_time_format(time_str): return True, None except (ValueError, TypeError): return False, f"Invalid time format: {time_str}. Expected HH:MM format." -def _validate_time_range(start_time_str, end_time_str, allow_overnight=True): - """Validate time range. Returns (is_valid, error_message)""" - try: - start_time = datetime.strptime(start_time_str, '%H:%M').time() - end_time = datetime.strptime(end_time_str, '%H:%M').time() - - # Allow overnight schedules (start > end) or same-day schedules - if not allow_overnight and start_time >= end_time: - return False, f"Start time ({start_time_str}) must be before end time ({end_time_str}) for same-day schedules" - - return True, None - except (ValueError, TypeError) as e: - return False, f"Invalid time format: {str(e)}" def _git_current_branch(project_dir): """Current branch name, or '' when detached or git fails.""" try: @@ -1380,6 +1366,15 @@ def _starlark_manifest_lock(): Callers should do their read, mutation and _write_starlark_manifest() call entirely inside the `with` block, mirroring the plugin's lock scope. """ + # Imported here, not at module scope: fcntl is POSIX-only, and a top-level + # import made the whole api_v3 package unimportable on Windows -- which the + # monolithic blueprint never was, so the split would have broken local dev + # and the test suite there. Deliberately NOT degraded to a no-op lock off + # POSIX: the docstring above describes a real lost-update race, and silently + # not locking would be worse than failing loudly on a platform that cannot + # run the display anyway. + import fcntl + _STARLARK_APPS_DIR.mkdir(parents=True, exist_ok=True) lock_fd = os.open(str(_STARLARK_MANIFEST_LOCK_FILE), os.O_CREAT | os.O_RDWR, 0o644) try: diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index 945bbcd0..a2bfaaa4 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -707,6 +707,35 @@ def reconcile_plugin_state(): context=error.context, status_code=500 ) +def _drop_stale_reconciliation_findings(unresolved): + """Re-check a stored reconciliation verdict against current state. + + The verdict is a snapshot written once per run, and a run that could not + apply a fix also refuses to retry -- so a resolved condition was reported + indefinitely. Best-effort: any failure here returns the list untouched, + because showing a stale warning beats failing the endpoint. + + The disk set is enumerated directly rather than through + _installed_plugin_ids(): this mirrors StateReconciliation._get_disk_state(), + whose verdict we are re-checking, and avoids triggering a discovery scan on + an endpoint the overview page polls. + """ + try: + from src.plugin_system.state_reconciliation import still_unresolved + + config_keys = set(api_v3.config_manager.load_config() or {}) + installed = set() + plugins_dir = getattr(api_v3.plugin_manager, 'plugins_dir', None) + if plugins_dir: + for entry in Path(plugins_dir).iterdir(): + if entry.is_dir() and (entry / 'manifest.json').exists(): + installed.add(entry.name) + return still_unresolved(unresolved, config_keys, installed) + except Exception: + logger.debug("[Reconciliation] Could not re-check stored findings", exc_info=True) + return unresolved + + @api_v3.route('/plugins/reconciliation-status', methods=['GET']) def get_reconciliation_status(): """Return the result of the last startup reconciliation from /tmp status file.""" @@ -721,6 +750,8 @@ def get_reconciliation_status(): try: with open(_recon_path) as _f: data = json.load(_f) + if data.get('unresolved'): + data['unresolved'] = _drop_stale_reconciliation_findings(data['unresolved']) return jsonify({'status': 'success', 'data': data}) except json.JSONDecodeError: logger.exception("[Reconciliation] Failed to parse status file: %s", _recon_path) diff --git a/web_interface/blueprints/api_v3/system.py b/web_interface/blueprints/api_v3/system.py index c991cdde..c9061faf 100644 --- a/web_interface/blueprints/api_v3/system.py +++ b/web_interface/blueprints/api_v3/system.py @@ -171,6 +171,33 @@ def check_for_update(): logger.warning("check-update failed: %s", e) return jsonify(_update_check_failed( "Could not check for updates; see logs for details.")) +#: sudo's own wording when it needs a password it cannot ask for. The web +#: interface runs unprivileged, so its systemctl/reboot/journalctl calls only +#: work once scripts/install/configure_web_sudo.sh has granted NOPASSWD -- +#: which first_time_install.sh does not do. That makes this the common case on +#: a fresh device, and "Action failed; see logs for details" named none of it, +#: while the log viewer was broken for the very same reason. +_SUDO_NEEDS_PASSWORD = ( + 'a password is required', + 'no tty present', + 'a terminal is required', +) + +_SUDO_HINT = ( + 'Passwordless sudo is not configured for the web interface user, so this ' + 'action cannot run. Run scripts/install/configure_web_sudo.sh as that user, ' + 'then retry.' +) + + +def _sudo_hint_for(text): + """An actionable hint when `text` is sudo refusing to prompt, else None.""" + lowered = (text or '').lower() + if any(marker in lowered for marker in _SUDO_NEEDS_PASSWORD): + return _SUDO_HINT + return None + + @api_v3.route('/system/action', methods=['POST']) def execute_system_action(): """Execute system actions (start/stop/reboot/etc)""" @@ -206,7 +233,14 @@ def execute_system_action(): logger.error("start_display (%s) stderr: %s", mode, result.stderr.strip()) resp = { 'status': 'success' if result.returncode == 0 else 'error', - 'message': 'Display started' if result.returncode == 0 else 'Failed to start display', + # This branch returns before the shared nonzero-result + # response below, so it needs the hint of its own or an + # on-demand start reports "Failed to start display" and + # says nothing about the sudo that actually refused it. + 'message': ( + 'Display started' if result.returncode == 0 + else _sudo_hint_for(result.stderr) or 'Failed to start display' + ), } if result.returncode != 0: resp['returncode'] = result.returncode @@ -538,6 +572,9 @@ def execute_system_action(): if result.returncode != 0: resp['returncode'] = result.returncode resp['stderr'] = result.stderr.strip() + hint = _sudo_hint_for(result.stderr) + if hint: + resp['message'] = hint return jsonify(resp) except subprocess.TimeoutExpired as e: @@ -545,7 +582,13 @@ def execute_system_action(): return jsonify({'status': 'error', 'message': 'Command timed out', 'returncode': -1, 'stderr': 'timeout'}) except Exception as e: logger.error("execute_system_action failed: %s", e, exc_info=True) - return jsonify({'status': 'error', 'message': 'Action failed; see logs for details'}), 500 + detail = describe_exception(e) + resp = { + 'status': 'error', + 'message': _sudo_hint_for(detail) or 'Action failed; see logs for details', + 'details': detail, + } + return jsonify(resp), 500 @api_v3.route('/system/git-info', methods=['GET']) def get_git_info(): """Return branch, dirty state, recent commits and remote URL for the Tools tab.""" diff --git a/web_interface/system_metrics.py b/web_interface/system_metrics.py new file mode 100644 index 00000000..723bd85f --- /dev/null +++ b/web_interface/system_metrics.py @@ -0,0 +1,62 @@ +"""System metrics for the status stream. + +Deliberately free of Flask and app imports: importing web_interface.app +constructs the Flask application and a CacheManager, and the latter claims the +cache directory with a cleanup thread. Reading a CPU percentage should not do +that, and neither should testing it. + +Every value is best-effort. A number that cannot be taken comes back as None so +the UI can render '--', because a confident wrong number is worse than a blank: +``disk_used_percent`` used to be hardcoded to 0, which read as "plenty of room" +on a card that was filling up. +""" + +from typing import Any, Dict, Optional + +_THERMAL_ZONE = '/sys/class/thermal/thermal_zone0/temp' + + +def _cpu_temp_c() -> Optional[float]: + """CPU temperature in degrees C, or None off a Raspberry Pi.""" + try: + with open(_THERMAL_ZONE, 'r') as f: + return round(float(f.read()) / 1000.0, 1) + except (OSError, ValueError): + return None + + +def collect_system_metrics() -> Dict[str, Any]: + """Collect the numbers that predict trouble on a small board. + + ``memory_available_mb`` is MemAvailable rather than a used percentage: it + accounts for reclaimable page cache, so it is what separates a board at 70% + "used" that is fine from one at 70% that is about to fail fork(). A 1GB Pi + can sit at either and only this number tells them apart. + """ + try: + import psutil + except ImportError: + return { + 'cpu_percent': 0, + 'memory_used_percent': 0, + 'memory_available_mb': None, + 'disk_used_percent': None, + 'cpu_temp': _cpu_temp_c() or 0, + } + + # interval=None is non-blocking; app startup primes psutil's internal state. + cpu_percent = round(psutil.cpu_percent(interval=None), 1) + memory = psutil.virtual_memory() + + try: + disk_used_percent: Optional[float] = round(psutil.disk_usage('/').percent, 1) + except OSError: + disk_used_percent = None + + return { + 'cpu_percent': cpu_percent, + 'memory_used_percent': round(memory.percent, 1), + 'memory_available_mb': round(memory.available / (1024 * 1024), 1), + 'disk_used_percent': disk_used_percent, + 'cpu_temp': _cpu_temp_c() or 0, + } diff --git a/web_interface/templates/v3/partials/overview.html b/web_interface/templates/v3/partials/overview.html index e351c90f..340fd35d 100644 --- a/web_interface/templates/v3/partials/overview.html +++ b/web_interface/templates/v3/partials/overview.html @@ -30,10 +30,33 @@ if (!d.unresolved || d.unresolved.length === 0) return; var key = d.unresolved.map(function (i) { return i.plugin_id; }).sort().join(','); if (sessionStorage.getItem(DISMISS_KEY) === key) return; - var ids = d.unresolved.map(function (i) { return i.plugin_id; }).join(', '); - document.getElementById('reconciliation-banner-text').textContent = - 'Stale plugin config entries found: ' + ids + - '. Remove them from config.json or reinstall via the Plugin Store.'; + // These are opposite problems and need opposite advice. Lumping + // them together told users to delete config.json entries for + // plugins that were installed and correctly configured. + function idsOfType(t) { + return d.unresolved.filter(function (i) { return i.type === t; }) + .map(function (i) { return i.plugin_id; }); + } + var notConfigured = idsOfType('plugin_missing_in_config'); + var notInstalled = idsOfType('plugin_missing_on_disk'); + var other = d.unresolved.filter(function (i) { + return i.type !== 'plugin_missing_in_config' + && i.type !== 'plugin_missing_on_disk'; + }).map(function (i) { return i.plugin_id; }); + + var parts = []; + if (notConfigured.length) { + parts.push('Installed but missing from config: ' + notConfigured.join(', ') + + '. Configure them in the Plugin Store - do not remove anything from config.json.'); + } + if (notInstalled.length) { + parts.push('In config but not installed: ' + notInstalled.join(', ') + + '. Reinstall via the Plugin Store, or remove these entries from config.json.'); + } + if (other.length) { + parts.push('Needs attention: ' + other.join(', ') + '.'); + } + document.getElementById('reconciliation-banner-text').textContent = parts.join(' '); var banner = document.getElementById('reconciliation-banner'); banner.dataset.dismissKey = key; banner.style.setProperty('display', 'flex', 'important');