diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d6e3c06..68713d0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,14 @@ accepts both, but the store flags the old spelling as deprecated ## Unreleased +- Fixes found testing on a Pi: + - Stopping `ledmatrix.service` runs the controller's cleanup (SIGTERM now takes the Ctrl-C path). + - The Logs tab's "Now showing" no longer reads "unknown" when one screen stays up longer than 2 minutes. + - Turning Vegas on in the web UI works without a restart when it was off at startup. + - `configure_web_sudo.sh` run as the web user keeps the reboot/poweroff rules. + - `check_system_compatibility.sh` no longer reports installed packages as missing. + - A network failure fetching GitHub repo info logs a warning, not an error. + - Scripts and installer: - `fix_web_permissions.sh` makes `safe_plugin_rm.sh` and `safe_pip_install.sh` root-owned again after resetting ownership. A web-user-owned copy of either is a root shell, since sudo lets the web user run them as root. It also restores `config_secrets.json` to mode 640. - `configure_wifi_permissions.sh` checks its rules with `visudo -c` before installing them, and grants the NetworkManager captive-portal `cp` and `rm` commands `wifi_manager` runs. diff --git a/scripts/check_system_compatibility.sh b/scripts/check_system_compatibility.sh index 22cc5050..87538e17 100755 --- a/scripts/check_system_compatibility.sh +++ b/scripts/check_system_compatibility.sh @@ -155,7 +155,10 @@ ESSENTIAL_PACKAGES=( for pkg_info in "${ESSENTIAL_PACKAGES[@]}"; do IFS=':' read -r pkg desc <<< "$pkg_info" - if dpkg -l | grep -q "^ii $pkg "; then + # dpkg-query rather than `dpkg -l | grep -q`: under pipefail, grep -q + # exiting on its first match kills dpkg with SIGPIPE and fails the pipeline, + # which reported installed packages as missing. + if [ "$(dpkg-query -W -f='${Status}' "$pkg" 2>/dev/null)" = "install ok installed" ]; then print_success "$desc ($pkg) is installed" else print_warning "$desc ($pkg) not installed - will be installed during setup" diff --git a/scripts/install/configure_web_sudo.sh b/scripts/install/configure_web_sudo.sh index c0fc0a06..bf82c354 100755 --- a/scripts/install/configure_web_sudo.sh +++ b/scripts/install/configure_web_sudo.sh @@ -26,11 +26,26 @@ fi # Get the full paths to commands and validate each one MISSING_CMDS=() -SYSTEMCTL_PATH=$(command -v systemctl) || true -REBOOT_PATH=$(command -v reboot) || true -POWEROFF_PATH=$(command -v poweroff) || true -BASH_PATH=$(command -v bash) || true -JOURNALCTL_PATH=$(command -v journalctl) || true +# Full path of a command, also looking in the sbin directories. This script runs +# as the web user, whose PATH usually lacks /usr/sbin and /sbin -- where reboot +# and poweroff live -- so `command -v` alone silently dropped their rules. +find_command() { + local found + found=$(command -v "$1" 2>/dev/null) && { printf '%s\n' "$found"; return 0; } + for dir in /usr/sbin /sbin /usr/bin /bin; do + if [ -x "$dir/$1" ]; then + printf '%s\n' "$dir/$1" + return 0 + fi + done + return 1 +} + +SYSTEMCTL_PATH=$(find_command systemctl) || true +REBOOT_PATH=$(find_command reboot) || true +POWEROFF_PATH=$(find_command poweroff) || true +BASH_PATH=$(find_command bash) || true +JOURNALCTL_PATH=$(find_command journalctl) || true SAFE_RM_PATH="$PROJECT_ROOT/scripts/fix_perms/safe_plugin_rm.sh" SAFE_PIP_INSTALL_PATH="$PROJECT_ROOT/scripts/fix_perms/safe_pip_install.sh" diff --git a/src/display_controller.py b/src/display_controller.py index 3f6222bf..847332be 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -23,6 +23,7 @@ Entry point: :func:`main` — instantiates :class:`DisplayController` and calls import time import os import inspect +import signal import json import threading import types @@ -44,6 +45,10 @@ from src.vegas_mode.render_pipeline import SYNC_SEND_INTERVAL # Get logger with consistent configuration logger = get_logger(__name__) +# How often the unchanged current mode is republished for the web UI, which +# treats display_current_state older than 120 s as unknown. +CURRENT_STATE_REFRESH_SECONDS = 30 + # How long startup will wait for plugins to fetch their first data before # showing anything. Each plugin's update blocks for up to the executor's 30s # timeout and they run one after another, so the uncapped total is the sum of @@ -216,6 +221,10 @@ class DisplayController: # the main run loop reconciles (loads/unloads) on its own thread so # mutating available_modes never races with rendering. self._pending_plugin_reconcile = False + # Set by the config-watcher thread when Vegas is switched on but no + # coordinator exists (Vegas was off at startup). The render thread + # creates it in _is_vegas_mode_active(), never the watcher thread. + self._pending_vegas_init = False # Monotonic stamp of the last mailbox disk read; see # _poll_on_demand_requests. None means "never polled", so the first # call always goes through. @@ -422,8 +431,9 @@ class DisplayController: # Display rotation state self.current_mode_index = 0 self.current_display_mode = None - # Last mode written to the display_current_state cache key. + # Last mode written to the display_current_state cache key, and when. self._last_published_mode: Optional[str] = None + self._last_published_at = 0.0 self.global_dynamic_config = ( self.config.get("display", {}).get("dynamic_duration", {}) or {} ) @@ -573,6 +583,7 @@ class DisplayController: def _is_vegas_mode_active(self) -> bool: """Check if Vegas mode should be running.""" + self._apply_pending_vegas_init() if not self.vegas_coordinator: return False # A stopped coordinator never reaches run_frame(), where queued config @@ -584,6 +595,19 @@ class DisplayController: return False # On-demand takes priority return True + def _apply_pending_vegas_init(self) -> None: + """Create the Vegas coordinator if Vegas was switched on after startup. + + Render thread only: the config watcher just sets _pending_vegas_init. + Called from _is_vegas_mode_active() and from the main loop before the + sync-follower branch, which skips _is_vegas_mode_active() while a + follower is connected but still needs the coordinator to show the + leader's scroll image. + """ + if not self.vegas_coordinator and self._pending_vegas_init: + self._pending_vegas_init = False + self._initialize_vegas_mode() + def _check_vegas_interrupt(self) -> bool: """ Check if Vegas should yield control for higher priority events. @@ -1190,13 +1214,22 @@ class DisplayController: } self.cache_manager.set('display_current_state', state) self._last_published_mode = self.current_display_mode + self._last_published_at = time.monotonic() except (OSError, RuntimeError, ValueError, TypeError) as err: logger.error("Failed to publish current display state: %s", err, exc_info=True) def _publish_current_mode_state_if_changed(self) -> None: - """Publish current mode state only when it actually changed, to avoid - writing to the shared cache on every render tick.""" - if self.current_display_mode != self._last_published_mode: + """Publish the current mode state when it changed, or when the last + publish is older than CURRENT_STATE_REFRESH_SECONDS. + + The web UI reads this key with a max_age (api_v3/display.py), so a mode + that stays on screen longer than that -- a live game under live + priority, a single enabled plugin -- has to be republished or the UI + reports it as unknown. Otherwise this writes only on a change, not on + every render tick. + """ + if (self.current_display_mode != self._last_published_mode + or time.monotonic() - self._last_published_at >= CURRENT_STATE_REFRESH_SECONDS): self._publish_current_mode_state() def _publish_on_demand_state(self) -> None: @@ -1304,6 +1337,9 @@ class DisplayController: self._check_on_demand_expiration() self._evaluate_schedule() self._apply_brightness_target(repaint=True) + # A Vegas iteration or a long screen keeps the main loop away for + # minutes; keep the web UI's "Now showing" from going stale. + self._publish_current_mode_state_if_changed() except Exception: # pylint: disable=broad-except # Called from inside Vegas and the render loops; a failure here # must not take the display loop down with it. @@ -1987,6 +2023,7 @@ class DisplayController: continue self._publish_current_mode_state_if_changed() + self._apply_pending_vegas_init() logger.debug("Display active, processing mode: %s", self.current_display_mode) # Plugins update on their own schedules - no forced sync updates needed @@ -3207,10 +3244,14 @@ class DisplayController: # new one only when it changed: applying it rebuilds the strip. # (getattr: this can fire before __init__ creates the coordinator.) vegas = getattr(self, 'vegas_coordinator', None) + new_vegas = (new_config.get('display', {}) or {}).get('vegas_scroll') if vegas is not None and ( - (old_config.get('display', {}) or {}).get('vegas_scroll') - != (new_config.get('display', {}) or {}).get('vegas_scroll')): + (old_config.get('display', {}) or {}).get('vegas_scroll') != new_vegas): vegas.update_config(new_config) + elif vegas is None and (new_vegas or {}).get('enabled', False): + # No coordinator yet because Vegas was off at startup. Creating + # one here would race the render thread; flag it instead. + self._pending_vegas_init = True # If a plugin was enabled/disabled, flag a reconcile for the main # loop to apply (loading/unloading off the watcher thread is unsafe). if (self._enabled_set_changed(old_config, new_config) @@ -3282,9 +3323,23 @@ class DisplayController: self.display_manager.cleanup() logger.info("Cleanup complete.") +def _raise_keyboard_interrupt(signum, frame): + """SIGTERM handler: stop the way Ctrl-C does. + + systemd stops ledmatrix.service with SIGTERM. Python's default action for + it ends the process at once, so run()'s ``finally: self.cleanup()`` -- + which stops the update worker, tears down Vegas and clears the panel -- + never ran. Raising KeyboardInterrupt sends SIGTERM down that same path. + """ + raise KeyboardInterrupt + + def main(): """Application entry point — create a DisplayController and run until interrupted.""" controller = DisplayController() + # Installed after construction: a SIGTERM while plugins are still loading + # keeps the default immediate exit rather than waiting for the loads. + signal.signal(signal.SIGTERM, _raise_keyboard_interrupt) controller.run() if __name__ == "__main__": diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index 1a2a2168..33f8ec86 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -623,6 +623,11 @@ class PluginStoreManager: return dict(self._EMPTY_REPO_INFO) + except requests.exceptions.RequestException as e: + # Offline, DNS or a timeout reaching GitHub: the listing still + # works without the extra repo info, so this is not an error. + self.logger.warning("GitHub repo info unavailable for %s: %s", repo_url, e) + return dict(self._EMPTY_REPO_INFO) except Exception as e: self.logger.error(f"Error fetching GitHub repo info for {repo_url}: {e}") return dict(self._EMPTY_REPO_INFO) diff --git a/test/test_display_controller_shutdown_and_state.py b/test/test_display_controller_shutdown_and_state.py new file mode 100644 index 00000000..f6449acb --- /dev/null +++ b/test/test_display_controller_shutdown_and_state.py @@ -0,0 +1,179 @@ +"""Display-controller fixes found testing on a real Pi (ledpi). + +- systemd stops the service with SIGTERM; it must run the same cleanup as Ctrl-C. +- The current mode is republished while it stays on screen, so the web UI's + "Now showing" does not turn into "unknown" after its 120 s max_age. +- Switching Vegas on in the web UI works when Vegas was off at startup. +""" + +import os +import signal +from unittest.mock import MagicMock, patch + +import pytest + +os.environ.setdefault("EMULATOR", "true") + +from src import display_controller as dc_module +from src.display_controller import DisplayController + + +# --- SIGTERM ---------------------------------------------------------------- + +def test_sigterm_handler_raises_keyboard_interrupt(): + with pytest.raises(KeyboardInterrupt): + dc_module._raise_keyboard_interrupt(signal.SIGTERM, None) + + +def test_main_routes_sigterm_through_run_cleanup(): + """main() installs the handler before run(); SIGTERM then ends run() the + way Ctrl-C does, so its finally-cleanup runs.""" + events = [] + + class FakeController: + def run(self): + try: + handler = signal.getsignal(signal.SIGTERM) + handler(signal.SIGTERM, None) # what the signal would do + except KeyboardInterrupt: + events.append("interrupted") + finally: + events.append("cleanup") + + previous = signal.getsignal(signal.SIGTERM) + try: + with patch.object(dc_module, "DisplayController", FakeController): + dc_module.main() + finally: + signal.signal(signal.SIGTERM, previous) + assert events == ["interrupted", "cleanup"] + + +# --- current-state heartbeat ------------------------------------------------ + +def _publisher(): + dc = object.__new__(DisplayController) + dc.cache_manager = MagicMock() + dc.current_display_mode = "mlb_live" + dc.mode_to_plugin_id = {"mlb_live": "baseball-scoreboard"} + dc.current_mode_index = 0 + dc.available_modes = ["mlb_live"] + dc.on_demand_active = False + dc.is_display_active = True + dc._last_published_mode = None + dc._last_published_at = 0.0 + return dc + + +def test_unchanged_mode_is_republished_after_the_refresh_interval(): + dc = _publisher() + now = [1000.0] + with patch.object(dc_module.time, "monotonic", lambda: now[0]): + dc._publish_current_mode_state_if_changed() # first publish + now[0] += 5 + dc._publish_current_mode_state_if_changed() # unchanged, recent + assert dc.cache_manager.set.call_count == 1 + now[0] += dc_module.CURRENT_STATE_REFRESH_SECONDS # same mode, stale + dc._publish_current_mode_state_if_changed() + assert dc.cache_manager.set.call_count == 2 + + +def test_refresh_interval_is_well_inside_the_web_max_age(): + # api_v3/display.py reads display_current_state with max_age=120. + assert dc_module.CURRENT_STATE_REFRESH_SECONDS < 120 / 2 + + +# --- Vegas switched on after startup ---------------------------------------- + +def _vegas_controller(enabled_at_start): + dc = object.__new__(DisplayController) + dc.config = {"display": {"vegas_scroll": {"enabled": enabled_at_start}}} + dc.vegas_coordinator = None + dc._pending_vegas_init = False + dc.on_demand_active = False + dc._refresh_config_cache = MagicMock() + dc._enabled_set_changed = MagicMock(return_value=False) + dc._enabled_plugin_not_running = MagicMock(return_value=False) + return dc + + +def test_enabling_vegas_live_creates_the_coordinator_on_the_render_thread(): + dc = _vegas_controller(enabled_at_start=False) + old = {"display": {"vegas_scroll": {"enabled": False}}} + new = {"display": {"vegas_scroll": {"enabled": True}}} + + dc._controller_config_change(old, new) # watcher thread: flag only + assert dc._pending_vegas_init is True + + coordinator = MagicMock(is_enabled=True) + + def init(): + dc.vegas_coordinator = coordinator + dc._initialize_vegas_mode = MagicMock(side_effect=init) + + assert dc._is_vegas_mode_active() is True # render thread: created here + dc._initialize_vegas_mode.assert_called_once_with() + assert dc._pending_vegas_init is False + dc._is_vegas_mode_active() + dc._initialize_vegas_mode.assert_called_once_with() # not re-created + + +def test_config_changes_without_vegas_enabled_do_not_flag_init(): + dc = _vegas_controller(enabled_at_start=False) + same = {"display": {"vegas_scroll": {"enabled": False}}} + dc._controller_config_change(same, same) + assert dc._pending_vegas_init is False + + +def test_sigterm_handler_is_installed_only_after_construction(): + """A SIGTERM while plugins load in __init__ keeps the default exit.""" + seen = {} + + class FakeController: + def __init__(self): + seen["during_init"] = signal.getsignal(signal.SIGTERM) + + def run(self): + seen["during_run"] = signal.getsignal(signal.SIGTERM) + + previous = signal.getsignal(signal.SIGTERM) + try: + signal.signal(signal.SIGTERM, signal.SIG_DFL) + with patch.object(dc_module, "DisplayController", FakeController): + dc_module.main() + finally: + signal.signal(signal.SIGTERM, previous) + assert seen["during_init"] is not dc_module._raise_keyboard_interrupt + assert seen["during_run"] is dc_module._raise_keyboard_interrupt + + +def test_service_pending_changes_keeps_the_state_fresh_during_long_renders(): + """Vegas iterations and long screens call _service_pending_changes, not + the main loop, for minutes at a time; it republishes a stale state.""" + dc = _publisher() + dc._last_pending_service = None + dc.PENDING_CHANGES_INTERVAL = 0.25 + for name in ("_poll_on_demand_requests", "_check_on_demand_expiration", + "_evaluate_schedule", "_apply_brightness_target"): + setattr(dc, name, MagicMock()) + now = [5000.0] + with patch.object(dc_module.time, "monotonic", lambda: now[0]): + dc._last_published_mode = "mlb_live" + dc._last_published_at = now[0] - dc_module.CURRENT_STATE_REFRESH_SECONDS - 1 + dc._service_pending_changes() + dc.cache_manager.set.assert_called_once() + + +def test_pending_vegas_init_is_applied_by_the_helper_the_main_loop_calls(): + dc = _vegas_controller(enabled_at_start=False) + dc._pending_vegas_init = True + dc._initialize_vegas_mode = MagicMock() + dc._apply_pending_vegas_init() + dc._initialize_vegas_mode.assert_called_once_with() + assert dc._pending_vegas_init is False + + +def test_main_loop_applies_pending_vegas_init_before_the_follower_branch(): + import inspect + src = inspect.getsource(DisplayController.run) + assert src.index("self._apply_pending_vegas_init()") < src.index("self.sync_manager.is_follower_active()") diff --git a/test/test_ledpi_script_fixes.py b/test/test_ledpi_script_fixes.py new file mode 100644 index 00000000..1cbb3348 --- /dev/null +++ b/test/test_ledpi_script_fixes.py @@ -0,0 +1,102 @@ +"""Script and logging fixes found testing on a real Pi (ledpi). + +- configure_web_sudo.sh runs as the web user, whose PATH lacks /usr/sbin, so + `command -v reboot` failed and the reboot/poweroff rules were dropped. +- check_system_compatibility.sh ran `dpkg -l | grep -q` under pipefail, which + reported installed packages as missing. +- A network failure fetching GitHub repo info is a WARNING, not an ERROR. +""" + +import logging +import re +import shutil +import subprocess +import sys +from pathlib import Path +from tempfile import TemporaryDirectory +from unittest.mock import patch + +import pytest +import requests + +ROOT = Path(__file__).resolve().parent.parent +WEB_SUDO = ROOT / "scripts" / "install" / "configure_web_sudo.sh" +COMPAT = ROOT / "scripts" / "check_system_compatibility.sh" + + +def _function_source(script: Path, name: str) -> str: + text = script.read_text(encoding="utf-8") + m = re.search(rf"^{name}\(\) \{{\n.*?^\}}\n", text, re.S | re.M) + assert m, f"{name}() not found in {script.name}" + return m.group(0) + + +needs_linux_bash = pytest.mark.skipif( + sys.platform == "win32" or not shutil.which("bash") or not Path("/bin/sh").exists(), + reason="runs the script's shell function; needs a Linux bash", +) + + +@needs_linux_bash +def test_find_command_looks_outside_path(): + """With a PATH that has none of the standard dirs, a command in one of + them is still found (reboot and poweroff live in /usr/sbin).""" + target = next((d for d in ("/usr/sbin", "/sbin", "/usr/bin", "/bin") + if Path(d, "sh").exists() or Path(d, "reboot").exists()), None) + name = "reboot" if Path(target, "reboot").exists() else "sh" + script = _function_source(WEB_SUDO, "find_command") + f'find_command {name}\n' + # Absolute bash: with PATH=/nonexistent, "bash" itself would not be found. + out = subprocess.run([shutil.which("bash"), "-c", script], env={"PATH": "/nonexistent"}, + capture_output=True, text=True) + assert out.returncode == 0, out.stderr + assert out.stdout.strip().endswith("/" + name) + + +def test_web_sudo_resolves_reboot_and_poweroff_through_find_command(): + text = WEB_SUDO.read_text(encoding="utf-8") + for var, cmd in (("REBOOT_PATH", "reboot"), ("POWEROFF_PATH", "poweroff")): + assert re.search(rf"^{var}=\$\(find_command {cmd}\)", text, re.M), var + assert "/usr/sbin" in _function_source(WEB_SUDO, "find_command") + + +def test_compat_check_does_not_pipe_dpkg_into_grep_q(): + # grep -q exits on its first match; dpkg then dies of SIGPIPE and, under + # `set -o pipefail`, the check fails for an installed package. + text = COMPAT.read_text(encoding="utf-8") + code = "\n".join(l for l in text.splitlines() if not l.lstrip().startswith("#")) + assert "pipefail" in code + assert not re.search(r"dpkg -l\s*\|\s*grep -q", code) + assert "dpkg-query -W" in code + + +@needs_linux_bash +def test_compat_package_check_reports_an_installed_package_as_installed(): + """Run the script's package test against a stub dpkg-query.""" + line = next(l for l in COMPAT.read_text(encoding="utf-8").splitlines() + if "dpkg-query -W" in l).strip() + with TemporaryDirectory() as tmp: + stub = Path(tmp, "dpkg-query") + # The package name is the last argument: dpkg-query -W -f=... . + stub.write_text('#!/bin/sh\nfor a; do last=$a; done\n' + '[ "$last" = "git" ] && printf "install ok installed"\nexit 0\n') + stub.chmod(0o755) + script = ("set -Eeuo pipefail\n" + f"for pkg in git notthere; do {line} echo \"$pkg:yes\"; else echo \"$pkg:no\"; fi; done\n") + out = subprocess.run(["bash", "-c", script], + env={"PATH": f"{tmp}:/usr/bin:/bin"}, capture_output=True, text=True) + assert out.returncode == 0, out.stderr + assert out.stdout.split() == ["git:yes", "notthere:no"] + + +def test_github_network_failure_logs_a_warning_not_an_error(caplog): + from src.plugin_system.store_manager import PluginStoreManager + with TemporaryDirectory() as tmp: + sm = PluginStoreManager(plugins_dir=tmp) + with patch("src.plugin_system.store_manager.requests.get", + side_effect=requests.ConnectionError("offline")), \ + caplog.at_level(logging.WARNING): + info = sm._get_github_repo_info("https://github.com/owner/repo") + assert info == dict(PluginStoreManager._EMPTY_REPO_INFO) + ours = [r for r in caplog.records if "owner/repo" in r.getMessage()] + assert ours, "nothing logged for the failed fetch" + assert all(r.levelno == logging.WARNING for r in ours), [(r.levelname, r.getMessage()) for r in ours]