fix: six bugs found testing main on a real Pi (#641)

* fix: six bugs found testing main on a real Pi (ledpi) - Stopping the service now runs cleanup. systemd stops ledmatrix.service with SIGTERM, whose default action ended Python before run()'s finally block, so the update worker, Vegas and the panel were never torn down. main() now turns SIGTERM into KeyboardInterrupt, the Ctrl-C path. - "Now showing" no longer turns into "unknown". display_current_state was only written on a mode change and the web UI reads it with max_age=120, so a live game or a single plugin on screen for longer read as unknown. It is republished every 30 s while unchanged. - Switching Vegas on in the web UI works when it was off at startup. The coordinator was only created at startup; the config watcher now flags it and the render thread creates it. - configure_web_sudo.sh finds reboot and poweroff in /usr/sbin. Run as the web user it could not, silently dropped their rules and still said it granted them, so the web UI's Reboot/Shutdown stopped working. - check_system_compatibility.sh reports installed packages as installed. `dpkg -l | grep -q` under pipefail failed when grep exited early. - A network failure fetching GitHub repo info logs a WARNING, not ERROR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(changelog): fixes found testing on a Pi Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: run the Linux-only script tests correctly The sbin-lookup test set PATH=/nonexistent and then could not find bash itself; call it by absolute path. The dpkg-query stub read $4, but the package name is the third (last) argument. Both now pass on a Pi. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(display): cover the follower, long-render and startup cases Review follow-ups on the ledpi fixes: - The pending Vegas start is applied before the sync-follower branch too (_apply_pending_vegas_init), which skips _is_vegas_mode_active() while a follower is connected but needs the coordinator for the leader's image. - _service_pending_changes(), which runs inside Vegas iterations and long screens, republishes a stale display_current_state as well; the main loop alone could be away for a 240 s Vegas iteration. - The SIGTERM handler is installed after DisplayController() is built, so a stop during parallel plugin loading keeps the default immediate exit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 14:25:08 +00:00 · 2026-09-27 18:13:49 -04:00
parent c4927e82a3
commit da5937da3d
7 changed files with 379 additions and 12 deletions
@@ -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()")
@@ -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=... <pkg>.
+        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]