From 2df273ecfc03815ce4f663bf0e9c08a854afe23a Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Thu, 10 Sep 2026 17:42:00 -0400 Subject: [PATCH 01/11] fix(web): default web_display_autostart to true, as the installer already does (#556) 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/config.template.json and first_time_install.sh ship the key as true, so the code default contradicted the shipped default in two places: absence means an older or hand-edited config, not a request to stay down. The failure mode 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 only trace was one journal line saying the flag was "false or not set" -- which reads as a deliberate setting rather than a missing key. Also start the web interface when config.json is missing or unparseable, instead of exiting. The web interface is how a config gets created and repaired, so a broken config is exactly when the user needs it most; leaving it down means there is no way back in. Only an explicit false/off disables autostart now, and the disabled message says "explicitly disabled" so the journal distinguishes a real setting from a default. Co-authored-by: Claude Opus 5 --- scripts/utils/start_web_conditionally.py | 42 +++++++++++----- test/test_web_autostart_default.py | 63 ++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 12 deletions(-) create mode 100644 test/test_web_autostart_default.py 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/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 From 12f3790994f24d4dce205a07b9f6c7aaf75ef04b Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:41:19 -0400 Subject: [PATCH 02/11] fix(install): render the systemd units from their templates, not from heredocs (#547) * fix(install): render the systemd units from their templates, not from heredocs The installers carried their own inline copies of units that also exist as templates under systemd/, and the copies drifted. install_service.sh renders ledmatrix.service from the template correctly, then wrote ledmatrix-web.service from a heredoc that predated it -- missing Wants=network-online.target, RestartSec=10, SyslogIdentifier, CacheDirectory, CacheDirectoryMode and Environment=USE_THREADING=1. install_web_service.sh had a third copy, and install_wifi_monitor.sh a fourth, that one already differing from its template (syslog where the template says journal). startup_validator.py compares the installed unit against the template, so a rig installed this way warned on every boot -- and the remedy the warning names, "re-run scripts/install/install_service.sh", reinstalled the same stale copy. The warning could never clear. Reproduced on a live rig running exactly that unit. All three installers now render systemd/*.service through the same placeholder substitution. The template gains a __USER__ placeholder rather than hardcoding User=root, because the web interface runs as whoever installed it. That last point was a second, independent cause of a permanent warning: the validator substituted a fixed "root", so any non-root install reported drift forever. It now reads User= from the installed unit -- an install-time decision, not something the template dictates -- and compares everything else strictly. first_time_install.sh already reads the installed User= the same way. Tests cover a non-root web unit not warning, a genuinely changed directive in that unit still warning, the User= fallback, and a grep-based guard that no installer under scripts/install/ contains an inline unit body. That guard is what found the install_wifi_monitor.sh copy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(install): escape sed replacements, use mktemp, and make render failures fatal Address CodeRabbit findings on install_service.sh, install_web_service.sh and install_wifi_monitor.sh: - Values interpolated into each script's sed expression (project root path, username) were not escaped, so a value containing &, \ or the | delimiter would corrupt the rendered systemd unit. Add a shared sed_escape_replacement() helper in the new scripts/install/lib_systemd_render.sh (sourced by all three scripts) and apply it to every sed replacement. - install_service.sh rendered the main and web units to the predictable path /tmp/ledmatrix.service.tmp before installing them -- a symlink/TOCTOU race (CWE-377). Use mktemp for both, with a trap to clean up on exit. - install_service.sh treated a missing template as a mere warning and then checked only whether a unit already existed at the destination before enabling/starting it, so a render failure could silently fall back to enabling a stale, previously-installed unit. Both unit blocks now exit non-zero on a missing template or a failed render. Also rename the ambiguous loop variable `l` to `line` in test/test_systemd_unit_drift.py (Ruff E741); ruff isn't wired into any CI workflow in this repo today, so this isn't currently CI-blocking, but the rename is trivial and correct regardless. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01S3bPMESe2TfrGvbs1ef9c5 * test(install): cover sed_escape_replacement against sed-special characters CodeRabbit asked for regression coverage using a project path containing an ampersand; the earlier commits on this branch already fixed the escaping, mktemp usage, and enable/start-on-fatal-render-failure findings, and the l->line rename was already applied -- this closes the one remaining gap. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 (1M context) --- scripts/install/install_service.sh | 98 ++++++++++++------- scripts/install/install_web_service.sh | 46 ++++----- scripts/install/install_wifi_monitor.sh | 34 +++---- scripts/install/lib_systemd_render.sh | 27 ++++++ src/startup_validator.py | 33 +++++-- systemd/ledmatrix-web.service | 16 ++-- test/test_systemd_unit_drift.py | 121 ++++++++++++++++++++++++ 7 files changed, 277 insertions(+), 98 deletions(-) create mode 100644 scripts/install/lib_systemd_render.sh 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/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/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_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") From 28bc79566ff2f2f09efd9739529cd7b9dfb1dca4 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:41:32 -0400 Subject: [PATCH 03/11] fix(logo): remember a missing logo instead of re-warning every rotation (#548) * fix(logo): remember a missing logo instead of re-warning every rotation load_logo() stat'd the path and logged a WARNING on every call, and the positive cache never covered it because a miss returns None and caches nothing. A file that is simply not there therefore produced one warning per rotation for as long as the process ran -- measured on a live rig at 114 lines in 24 hours for a single missing ticker icon, for a file nobody was going to add. Misses are now remembered for 10 minutes: warn once, then return None without touching the disk. Bounded rather than permanent because logo_downloader writes logos at runtime, so a file that appears later must still be picked up without a restart. Downloads through load_logo_with_download() clear the entry outright -- load_logo() consults the miss record before it stats the disk, so without that a freshly downloaded logo would stay invisible for the whole window. This is in the core rather than in ledmatrix-stocks, where it was found, so every plugin that goes through LogoHelper gets it. _cache_order stays a list. Swapping the pair for an OrderedDict would shave an O(n) scan per cache hit, but n is capped at cache_size (100 by default) and test_logo_helper.py pins the current structure; not worth the churn. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(logo): make the miss TTL longer than the rotation it is meant to outlast Deployed the previous commit to a live rig and measured it: no change at all. "Logo not found for VOO" stayed at ~6 lines an hour, exactly the baseline. The TTL was 600s and the display rotation is ~618s, so every recheck expired just as the plugin came round again and the negative cache never once got to suppress a warning. The fix was correct in shape and useless in practice, which only measuring on the rig would show. An hour instead. That is safe because the TTL is not the main way an entry clears: load_logo_with_download() drops it the moment a download succeeds and clear_cache() drops all of them. The TTL only covers a file that appeared some other way -- someone copying one in by hand -- and 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. The general lesson is in the comment: a TTL has to be long relative to the loop that does the asking, not merely "a while". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 --------- Co-authored-by: Claude Opus 5 (1M context) --- src/common/logo_helper.py | 40 ++++++++++++++++++++++ test/test_logo_helper.py | 70 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 1 deletion(-) 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/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 == {} From 8da13f02f81a1b83e7b547e5d8a1bfd53da66ba6 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:41:44 -0400 Subject: [PATCH 04/11] chore: ignore team logos fetched at runtime (#551) 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 accumulates untracked files nobody intended to commit. This checkout had 62; hdpi shows the same. The cost is not the files, it is that a permanently dirty `git status` trains everyone to ignore the one signal that says a checkout is not what you think it is. That is how a stale tree sat unnoticed on a rig for hours until a restart surfaced four sports plugins that could no longer import. Ignoring a directory does not untrack what is already in it, so the logos that ship keep shipping -- verified: 209 and 153 still tracked, no deletions in the diff. Only new downloads are hidden. Adding a logo on purpose stays possible and is what the escape hatch in the comment documents. It is also rare: the last deliberate addition was #415, four named NCAA logos a plugin needed, and `git log` finds no other in a year. So the common case is noise and the rare case is explicit, which is the right way round. Untracked files: 62 -> 0. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) --- .gitignore | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) 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/ From fb3b293ace64df31e478a47628be0d65d6790905 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:41:58 -0400 Subject: [PATCH 05/11] fix(plugins): let a plugin ask to be polled faster while it has live content (#555) * fix(plugins): let a plugin ask to be polled faster while it has live content Reported: "the football plugin with live games only updates the live game in progress if I restart the display." The data path was never the problem. NFLLiveManager fetches ESPN with no cache, SportsLive.update() refreshes current_game in place when the game IDs are unchanged, and the scorebug redraws from the game dict every frame -- which is why the reporter's logs look healthy. The problem is cadence. _get_plugin_update_interval() read only the manifest's static update_interval, football's manifest pins that to 60, and the plugin's own live_update_interval (15s) was invisible to the scheduler. Measured on a rig during the fourth quarter of the game in the report: 23:21:49 23:22:50 23:23:50 23:24:50 23:25:50 <- exactly 60s apart A clock and score up to a minute stale during a two-minute drill reads as a frozen panel, and a restart is the one moment it is ever current. A single static number cannot say "every 15 seconds while a game is on, every 15 minutes in July", and only the plugin knows which is true. get_update_interval() lets it say so per tick; returning None means "no opinion" and the existing manifest/config resolution applies, so every plugin that predates this is unaffected. Requests are clamped to MIN_DYNAMIC_UPDATE_INTERVAL (5s): a plugin returning 0 would otherwise be re-entered on every tick of the render loop, busy-waiting against its own API. A hook that raises or returns a non-number is ignored rather than propagated -- a scheduler that fails on one plugin's bug stops updating all the others. Deliberately NOT changed: the manifest still beats config in the static path. That looked like the obvious fix -- user config being silently ignored -- until checking a real rig, where football and baseball both carry update_interval 3600 in config against a manifest 60, and weather 1800 against 60. Those values are stale precisely because nothing has been honouring them; making config win would have slowed three plugins by 60x, turning a one-minute lag into an hour. The dynamic hook makes the flip unnecessary. There is a test pinning the current precedence with that reasoning attached. Full suite: 4,283 passed, 68 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * test(plugins): drive the real scheduler, not just the interval resolver test_plugin_dynamic_update_interval.py asserts that _get_plugin_update_interval() returns the number the plugin asked for. 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 ticks the real run_scheduled_updates() through a simulated hour and counts dispatches. Against pre-fix core it reports "10 updates in 10 minutes of a live game" -- the 60s manifest cadence, matching what was measured on a rig during the reported game. Against the fix it reports ~40. Also pins the regression that would be worse than the bug: an idle hour must still be ~60 updates, not 240. Asking for the live interval year-round would poll ESPN four times a minute all summer. Scope note, since it is easy to over-read this fix: the *switch* display path already refreshed the manager immediately before drawing, via _try_manager_display() -> _ensure_manager_updated(), which honours the manager's own 15s interval. So a switch-mode card was already <=15s stale at draw time before this change. What this fixes is the background cadence, which is what live-priority detection, Vegas content and scroll preparation all read. Full suite: 4,288 passed, 68 skipped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 * fix(plugins): reject bool and -inf hook results in dynamic interval get_update_interval() ran bool through float() (bool is an int subclass, so True/False became 1.0/0.0) and only checked for +inf, not -inf. Both cases landed on the MIN_DYNAMIC_UPDATE_INTERVAL floor by coincidence instead of falling back to the static/manifest interval as invalid input should. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017Tst9cied2ri9bH4QRWa6H --------- Co-authored-by: Claude Opus 5 (1M context) --- src/plugin_system/base_plugin.py | 32 ++++ src/plugin_system/plugin_manager.py | 62 +++++++- test/test_live_update_cadence_integration.py | 159 +++++++++++++++++++ test/test_plugin_dynamic_update_interval.py | 153 ++++++++++++++++++ 4 files changed, 402 insertions(+), 4 deletions(-) create mode 100644 test/test_live_update_cadence_integration.py create mode 100644 test/test_plugin_dynamic_update_interval.py 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_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/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_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 From ad5bc4b8190a3f2b774fbf089716b5a488861150 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:42:18 -0400 Subject: [PATCH 06/11] perf(sports): LRU-bound the decoded logo cache (#559) SportsCore._logo_cache was a plain dict keyed by team abbreviation with no eviction. Its entries are not file bytes but decoded RGBA thumbnails sized to display*1.5 -- roughly 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 held the whole league resident: about 11-18MB per manager instance, and a league runs three (live/recent/upcoming) that each keep their own cache, so the same logos were duplicated across them. On the 1GB Pi 3B+ this was measured on, one board was sitting at 439MB resident with ~290MB available, so tens of megabytes of duplicated league logos is real money. Bounded to 64 entries, which holds a full "other games" cycle (on the order of 20 games, 40 teams) without thrashing while capping the cache well below a 307-team league. Eviction is LRU rather than clear-when-full, using the OrderedDict/popitem pattern the neighbouring caches in this codebase already use (_IMAGE_CACHE_MAX, _FIT_CACHE_MAX, _TEXT_WIDTH_CACHE_MAX). That ordering matters: the logos on screen right now are precisely the ones that must not be discarded, so a cache hit moves the entry to the end. Co-authored-by: Claude Opus 5 --- src/base_classes/sports/core.py | 18 +++- test/test_sports_logo_cache_bounded.py | 120 +++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 test/test_sports_logo_cache_bounded.py 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/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 From dcd6e39c96eac4fa0aafed53c2d846551cb118a5 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:42:30 -0400 Subject: [PATCH 07/11] fix(web): report real disk usage and MemAvailable on the live status stream (#558) The SSE status stream sent 'disk_used_percent': 0 as a literal, so every consumer of the live view showed 0% disk no matter how full the card was. /api/v3/system/status computed it correctly; the stream that the dashboard actually watches did not. On a Pi with a modest SD card that is the warning a user most needs, and it was guaranteed to never appear. The stream also omitted memory_available_mb. /api/v3/system/status carries it with a comment spelling out why it matters: MemAvailable accounts for reclaimable page cache, so it is what separates a board reading 70% "used" that is fine from one reading 70% that is about to fail fork(). A 1GB Pi 3B+ can sit at either. The number that predicts the failure was missing from the live view. An unreadable disk now reports None rather than 0. The UI already renders null as '--'; a confident 0 reads as "plenty of room", which is worse than a blank. Metric collection moves to web_interface/system_metrics.py, with no Flask or app imports. That is not cosmetic: importing web_interface.app constructs the Flask application and a CacheManager, and the latter claims the cache directory with a cleanup thread. The first version of these tests imported the generator directly and broke test_cache_cleanup_thread_ownership ("one thread per directory") plus four starlark route tests through that side effect. Reading a CPU percentage should not boot a web application, and testing it should not either. Co-authored-by: Claude Opus 5 --- test/test_live_status_fields.py | 103 ++++++++++++++++++++++++++++++++ web_interface/app.py | 31 ++++------ web_interface/system_metrics.py | 62 +++++++++++++++++++ 3 files changed, 175 insertions(+), 21 deletions(-) create mode 100644 test/test_live_status_fields.py create mode 100644 web_interface/system_metrics.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/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/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, + } From 577f5501a698933e402cbe3c65f7b8ba8ee1bcae Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:42:49 -0400 Subject: [PATCH 08/11] perf(plugins): stop re-deriving a display() signature the caller already cached (#549) display_controller resolves once, and caches, whether a plugin's display() takes a display_mode keyword -- self._plugin_accepts_display_mode, populated right before the dispatch. It then handed the executor a types.SimpleNamespace wrapping a closure, and execute_display() ran inspect.signature() on that to work out the same thing. Because the SimpleNamespace is rebuilt per call, the callable was new every time, so nothing inside the executor could ever cache it either. Measured at ~39us per dispatch on a Pi 4, for a value the caller had a line earlier. execute_display() now takes accepts_display_mode, falling back to inspecting only when a caller does not pass it, so existing callers are unaffected. Also documents two things that read as bugs and are not: - execute_with_timeout()'s timeout is advisory. Nothing cancels the thread -- Python cannot -- so on expiry the operation runs to completion in the background and only the caller gives up. A permanently hung plugin leaks a daemon thread per attempt. This is why callers holding a lock across the call must release it from inside the wrapped callable, as run()'s _release_display_lock already does. - Only the first display() of each mode goes through the executor; the per-frame loops call display() directly. That is deliberate: a thread per frame would cost more than an advisory timeout buys. Both loops now say so, so the asymmetry does not read as an oversight. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) --- src/display_controller.py | 27 +++++++++++++++++++++++- src/plugin_system/plugin_executor.py | 31 +++++++++++++++++++++++----- 2 files changed, 52 insertions(+), 6 deletions(-) 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/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: From aba96e25b3a4a196ddeef7e8fa492f042192d575 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:45:07 -0400 Subject: [PATCH 09/11] chore: delete three functions nothing calls (#550) src/base_classes/baseball.py _get_baseball_display_text 45 lines src/web_interface/api_helpers.py validate_request_params 22 web_interface/blueprints/api_v3.py _validate_time_range 14 Each has exactly one occurrence across both repositories -- its own definition. No decorator, no __all__, no getattr dispatch, nothing in templates or JavaScript. A fourth candidate was dropped after checking: _unshare_element_fonts in src/common/sports_shared.py looked unreferenced, but eight scoreboard plugins call SportsCore._unshare_element_fonts directly from their test_element_text_colors.py, plus their own copies at runtime. It is live API. The earlier reading came from a plugins checkout 84 commits behind main, which is a good argument for re-verifying this kind of claim against a fresh tree rather than trusting an earlier scan. Full suite: 4,265 passed, 68 skipped. Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9 Co-authored-by: Claude Opus 5 (1M context) --- src/base_classes/baseball.py | 45 ------------------------------ src/web_interface/api_helpers.py | 24 ---------------- web_interface/blueprints/api_v3.py | 14 ---------- 3 files changed, 83 deletions(-) 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/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/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 865789f0..d5aeae3c 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -352,20 +352,6 @@ def _validate_time_format(time_str): 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)}" - @api_v3.route('/config/schedule', methods=['POST']) def save_schedule_config(): """Save schedule configuration""" From 39f27d285da25d32b47f4ed4c48e30072f393ab8 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:45:21 -0400 Subject: [PATCH 10/11] fix(plugins): stop reconciliation inventing plugins and telling users to delete real config (#557) On a device running four installed, configured, working plugins, the overview banner read: Stale plugin config entries found: football-scoreboard, odds-ticker, data, ledmatrix-weather, starlark-apps. Remove them from config.json or reinstall via the Plugin Store. Every claim in that sentence was wrong, and following its advice would have deleted 4.9KB of working league settings. Four separate defects combined. 1. Secrets keys became phantom plugins. load_config() merges config_secrets.json into the config it returns, and the ignore list named only 'github' and 'youtube'. A 'data' key in that file therefore read as a plugin id and was reported as "in config but not on disk" forever. Read the secrets file's own top-level keys instead of hardcoding two of them. 2. The auto-fix clobbered real config. The handler for "on disk but not in config" assigned `config[plugin_id] = {'enabled': False}` unconditionally, so whenever detection was wrong it replaced a plugin's entire configuration with a stub. On the reported device it only failed to do so because the write hit EACCES. Now it refuses to overwrite an entry that already exists. 3. The banner gave backwards advice. plugin_missing_in_config ("on disk, not in config") and plugin_missing_on_disk ("in config, not on disk") are opposite problems, and both were rendered as "stale config entries ... remove them from config.json" -- which is correct for the second and destructive for the first. They are now reported separately, each with the advice that fits. 4. A stale verdict was served indefinitely. The result is a snapshot written once per run to a status file, and a run that fails to apply a fix also declares it will not retry. A condition that had since resolved kept being reported for hours. The status endpoint now re-checks stored findings against current state, dropping only what it can prove stale and keeping any kind it cannot re-verify. The secrets-key lookup is deliberately fail-safe: an unreadable, absent, malformed or non-path secrets location narrows the ignore set rather than raising. An earlier revision let TypeError escape, which the broad handler in _get_config_state() swallowed as "Error reading config state" -- emptying the config state and making every downstream detection wrong. The existing reconciliation tests caught it; there is now a regression test for it too. Co-authored-by: Claude Opus 5 --- src/plugin_system/state_reconciliation.py | 66 ++++++- test/test_reconciliation_phantom_plugins.py | 170 ++++++++++++++++++ web_interface/blueprints/api_v3.py | 26 +++ .../templates/v3/partials/overview.html | 31 +++- 4 files changed, 287 insertions(+), 6 deletions(-) create mode 100644 test/test_reconciliation_phantom_plugins.py 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/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/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index d5aeae3c..a42d712f 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -3464,6 +3464,30 @@ def reconcile_plugin_state(): 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. + """ + 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.""" @@ -3478,6 +3502,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/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'); From 0ab95586fb89c5a57f61d2365c914bbc43012943 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:45:40 -0400 Subject: [PATCH 11/11] fix(web): say when a system action failed for want of passwordless sudo (#560) * fix(web): say when a system action failed for want of passwordless sudo POSTing reboot_system to a Pi returns, in full: {"message": "Action failed; see logs for details", "status": "error"} The cause is that the web interface runs unprivileged, and its systemctl/reboot/journalctl calls only work once scripts/install/configure_web_sudo.sh has granted NOPASSWD. first_time_install.sh never invokes that script and no user-facing doc mentions it, so on a fresh device every privileged action fails -- start_display, stop_display, the autostart toggles, reboot, and the log viewer. That last one closes the loop: "see logs for details" is unreachable advice when journalctl is refused for the same reason. This is exactly the failure src/web_interface/error_handler.py's describe_exception() was written to break, and /system/action's exception handler was still discarding the cause instead of using the helper the module already imports. Two changes, no behaviour change when things work: - The exception path now returns 'details': describe_exception(e), matching how the other handlers in this blueprint already report. - A failure whose stderr or exception text is sudo refusing to prompt ("a password is required", "no tty present", "a terminal is required") reports what to do about it, naming configure_web_sudo.sh. Unrelated failures keep the generic message and their stderr, so a missing unit is not blamed on sudo. Granting the sudo rights is left alone deliberately: auto-running a script that hands out NOPASSWD is a security decision for the maintainer, not something to slip into an installer. Making the refusal legible is the part that is unambiguously an improvement. Co-Authored-By: Claude Opus 5 * fix(web): apply the sudo hint on the on-demand start_display path too start_display with a mode builds its own response and returns before the shared nonzero-result path, so a recognized sudo refusal there reported only "Failed to start display" and said nothing about the passwordless sudo that refused it -- the exact gap the rest of this PR closes everywhere else. Raised by CodeRabbit on #560 and verified against the code before fixing: the branch at api_v3.py:2058 does return early past the shared handler. Three regression cases: the on-demand branch reports the sudo cause, keeps its "Display started" message on success, and does not blame an unrelated failure on sudo. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- test/test_system_action_sudo_diagnosis.py | 131 ++++++++++++++++++++++ web_interface/blueprints/api_v3.py | 47 +++++++- 2 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 test/test_system_action_sudo_diagnosis.py 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/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index a42d712f..10711053 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -1979,6 +1979,33 @@ def check_for_update(): 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)""" @@ -2014,7 +2041,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 @@ -2346,6 +2380,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: @@ -2353,7 +2390,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():