Files
LEDMatrix/test/test_systemd_unit_drift.py
T
ChuckandClaude Opus 5 12f3790994 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) <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-09-11 08:41:19 -04:00

274 lines
12 KiB
Python

"""An installed unit that no longer matches the repo's must be reported.
Nothing re-applies systemd units after the first install. `git pull` -- what
the web UI's update button runs -- brings a new template into the checkout, but
no code in web_interface/ or src/ copies it to /etc/systemd/system or runs
`systemctl daemon-reload`. The unit that actually runs is whatever
first_time_install.sh wrote on day one.
So every hardening added to a unit is inert on existing installs. Measured on a
live rig: the installed unit was dated 2026-08-06 and the repo's 2026-08-19,
and they differed -- with the result that a MemoryMax=85% present in the repo's
template was not being enforced at all. `systemctl show` reported
MemoryMax=infinity.
This is a warning, not an error, and deliberately not a silent rewrite:
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
import pytest
from src.startup_validator import StartupValidator
@pytest.fixture
def validator():
v = StartupValidator(config_manager=MagicMock())
v.logger = logging.getLogger("test")
v.warnings = []
v.errors = []
return v
def test_a_matching_unit_produces_no_warning(validator, tmp_path):
"""The installed unit, substituted exactly as the installer would."""
project_root = Path("src/startup_validator.py").resolve().parent.parent
template_rel = "systemd/ledmatrix.service"
template = project_root / template_rel
if not template.is_file():
pytest.skip("repo unit template not present")
installed = tmp_path / "ledmatrix.service"
installed.write_text(
template.read_text(encoding="utf-8")
.replace("__PROJECT_ROOT_DIR__", str(project_root))
.replace("__USER__", "root"),
encoding="utf-8")
validator._UNITS = ((template_rel, str(installed)),)
validator._validate_systemd_units()
assert not validator.warnings, f"a matching unit warned: {validator.warnings}"
assert not validator.errors
def test_comments_and_blank_lines_are_not_drift():
"""Otherwise every comment the repo adds would look like a changed unit."""
a = "[Service]\n# explains a setting\nExecStart=/x\nRestart=always\n"
b = "[Service]\nExecStart=/x\n\nRestart=always\n"
assert StartupValidator._unit_body(a) == StartupValidator._unit_body(b)
def test_a_changed_directive_is_drift():
a = "[Service]\nExecStart=/x\nMemoryMax=85%\n"
b = "[Service]\nExecStart=/x\n"
assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b)
def test_reordered_directives_are_drift():
"""Order is not noise in a systemd unit.
Repeated directives -- ExecStartPre=, ExecStartPost= -- run in the order
they appear, and a directive that moves between [Unit], [Service] and
[Install] means something different, or nothing, where it lands. This
check used to sort the lines before comparing, which reported no drift for
a unit that had genuinely changed.
"""
a = "[Service]\nExecStartPre=/first\nExecStartPre=/second\n"
b = "[Service]\nExecStartPre=/second\nExecStartPre=/first\n"
assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b), (
"swapping two ExecStartPre= lines changes what runs first, and was "
"being normalised away")
def test_a_directive_moved_between_sections_is_drift():
a = "[Unit]\nDescription=x\n[Service]\nExecStart=/x\n"
b = "[Unit]\nDescription=x\nExecStart=/x\n[Service]\n"
assert StartupValidator._unit_body(a) != StartupValidator._unit_body(b), (
"ExecStart= in [Unit] is not the same unit, and sorting hid it")
def test_cosmetic_differences_do_not_warn(validator, tmp_path):
"""Through the real comparison, not the helper.
The repo's template carries explanatory comments the installed copy may not
have, and the installer does not preserve ordering or blank lines. If those
counted as drift, every boot would warn and the warning would be ignored.
Asserting this on _unit_body alone would not catch a comparison that stopped
calling it -- which is exactly what a careless edit does.
"""
project_root = Path("src/startup_validator.py").resolve().parent.parent
template_rel = "systemd/ledmatrix.service"
template = project_root / template_rel
if not template.is_file():
pytest.skip("repo unit template not present")
substituted = (template.read_text(encoding="utf-8")
.replace("__PROJECT_ROOT_DIR__", str(project_root))
.replace("__USER__", "root"))
# Cosmetic means comments, blank lines and stray indentation -- the things
# the installer really does drop. Not reordering: that changes the unit,
# and is asserted as drift above.
directives = [line.strip() for line in substituted.splitlines()
if line.strip() and not line.strip().startswith("#")]
installed = tmp_path / "ledmatrix.service"
installed.write_text(
"\n\n".join(" " + d for d in directives) + "\n", encoding="utf-8")
validator._UNITS = ((template_rel, str(installed)),)
validator._validate_systemd_units()
assert not validator.warnings, (
f"cosmetic-only difference reported as drift: {validator.warnings}")
def test_drift_is_reported_as_a_warning(validator, tmp_path):
"""The whole point: a real difference must surface, and only as a warning."""
installed = tmp_path / "ledmatrix.service"
installed.write_text("[Service]\nExecStart=/usr/bin/python3 /x/run.py\n")
project_root = Path("src/startup_validator.py").resolve().parent.parent
template_rel = "systemd/ledmatrix.service"
template = project_root / template_rel
if not template.is_file():
pytest.skip("repo unit template not present")
validator._UNITS = ((template_rel, str(installed)),)
validator._validate_systemd_units()
assert validator.warnings, "a differing unit produced no warning"
assert "install_service.sh" in validator.warnings[0], (
"the warning does not tell the user how to fix it")
assert not validator.errors, "drift must not be fatal at startup"
def test_a_missing_installed_unit_is_silent(validator, tmp_path):
"""Development checkouts have no /etc/systemd unit; that is not drift."""
validator._UNITS = (("systemd/ledmatrix.service", str(tmp_path / "absent.service")),)
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")