mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-21 18:39:06 +00:00
* perf(systemd): cap glibc malloc arenas on the display service
Measured on a live rig 2.5 hours after start:
RSS 1030 MB
Private_Dirty 988 MB
anonymous mappings > 10 MB 23
largest 104, 79, 66, 63, 63 MB, on 64 MB-aligned addresses
threads 9
cores 3 -> glibc ceiling = 8 x 3 = 24 arenas
23 against a ceiling of 24, all 64 MB-aligned: these are glibc's per-thread
malloc arenas, not live objects. The data the process was actually holding
accounts for perhaps 15 MB -- the widest scroll strip observed was 35,746 x 64,
about 7 MB as RGB and the same again for its numpy mirror.
It is bloat rather than a leak: sampled four times over 135 seconds, RSS sat
between 990 and 1030 MB rather than climbing. glibc gives each allocating
thread its own arena, grows them to hold peak demand, and never gives them
back. A process that builds and drops large images across several threads is
exactly the shape that produces this.
The device had 59 MB free at the time, on 1845 MB total.
MALLOC_ARENA_MAX=2 trades a little allocator concurrency for that resident
memory. It is a tuning knob rather than a fix for a defect, so the rationale
and the measurements sit next to it in the unit file, and a test asserts they
stay there -- a bare environment variable invites removal by whoever meets it
next.
Two things this is NOT, both checked rather than assumed:
- Not an OOM problem today. A grep for "oom" in the service journal returned
24 matches, all of which were the radar logging zoom=9 and zoom=7. The kernel
OOM killer has not fired: dmesg has zero matches.
- Not currently capped by the unit's MemoryMax=85% either. That directive is in
this file but absent from the unit actually installed on the rig, which
reports MemoryMax=infinity, so nothing is enforcing a ceiling there.
The saving is unmeasured on hardware: applying it needs a service restart,
which blanks the panel, so that is the user's call rather than something to do
mid-audit. If p99 frame time regresses -- it sits at 18.4 ms against a 16.7 ms
budget for 60 FPS, so there is not much headroom -- raise the value rather than
remove it.
(cherry picked from commit 446207ffbc)
* test(systemd): pin the arena value instead of accepting a range
Review follow-up. The range check accepted 1, 3 and 4, so a change to 4 --
which hands most of the resident saving back -- passed a test whose whole
purpose is to notice that.
Pinned to the value the unit ships, in one named constant. Raising it is still
a legitimate response to a frame-time regression, but it should be a visible
edit here rather than silent drift, and the failure message says so.
Mutation-checked: changing the unit to 4 now fails.
(cherry picked from commit 73fff8d2d5)
* fix(startup): warn when an installed systemd unit has drifted from the repo's
Nothing re-applies systemd units after the first install. `git pull` -- which
is 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, and nothing anywhere 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, silently.
Measured on a live rig:
installed /etc/systemd/system/ledmatrix.service 2026-08-06
template systemd/ledmatrix.service 2026-08-19
contents differ
with the practical result that the MemoryMax=85% the repo's template specifies
was not being enforced at all -- `systemctl show` reported
MemoryMax=infinity. Anyone reading the template would reasonably believe the
service was capped.
Startup now compares each installed unit against its substituted template and
warns when they differ, naming install_service.sh as the remedy.
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 is booting. Making it fatal
would also brick every development checkout whose unit is legitimately absent
or hand-edited.
Comparison ignores comments, blank lines and ordering. The template carries
explanatory comments the installed copy will not have, and systemd does not
care about order within a section, so a literal comparison would warn on every
boot and be ignored within a week.
Mutation-checked three ways: never reporting drift fails, making it fatal
fails, and -- after the first attempt missed it -- comparing raw text now fails
too. That last gap is worth noting: the comment-insensitivity tests originally
exercised the helper directly, so a comparison that stopped calling the helper
passed them all. The test that catches it goes through _validate_systemd_units.
29 startup-validator tests pass.
(cherry picked from commit cf521bdfd8)
* fix(install): grant the sudo commands the captive portal actually runs
The installers write two allow-lists, /etc/sudoers.d/ledmatrix_web and
ledmatrix_wifi. Anything the code runs under sudo that is not in one of them
needs a password, which a service cannot supply, so the call fails.
Five commands were being run and none of them granted:
sysctl -w net.ipv4.ip_forward=0|1 wifi_manager.py:788, 883
nft add|delete table ip ledmatrix wifi_manager.py:835, 895
rfkill unblock wifi wifi_manager.py:1811
iptables ... wifi_manager.py:796, 813, 818, 871
mkdir -p .../dnsmasq-shared.d wifi_manager.py:922
Together these are the captive portal: unblock the radio, bring up the AP,
add the redirect, turn on forwarding, and undo all of it afterwards. Without
the grants a hardened install would associate clients to the access point and
then fail to route them.
Why it has gone unnoticed: a stock Raspberry Pi image ships
/etc/sudoers.d/010_pi-nopasswd granting the default user
<user> ALL=(ALL) NOPASSWD: ALL
which satisfies every one of these regardless of what the allow-lists say.
Confirmed on a live rig -- `sudo -n -l` permits sysctl there, and the blanket
rule is why. The allow-lists are effectively decorative on a default image and
only start mattering once that rule is removed or the service runs as another
user.
test_sudo_allowlist_covers_calls.py extracts every argv-style sudo call in
src/ and web_interface/ and asserts an installer grants it, so the next command
added without a rule fails here rather than on someone's hardened box.
Getting that test honest took three passes, each worth recording:
- Matching the literal "systemctl" against rules written as
`$SYSTEMCTL_PATH enable ...` reported six gaps that did not exist. Binary
path variables are now normalised before comparing.
- Scanning the whole installer let `NFT_PATH=$(command -v nft)` -- a variable
definition, not a grant -- satisfy the check on its own, so deleting the
actual nft rules still passed. Only NOPASSWD lines are considered now.
- `sudo -n <tool>` reported "-n" as the binary. sudo's own flags are skipped.
Each of the five grants is individually mutation-checked: removing any one
fails the suite.
(cherry picked from commit a372b43cd1)
* fix(install): drop the iptables wildcard, and pin each grant properly
Review follow-up. Two findings, both right, and the first is a hole I opened
myself.
`NOPASSWD: iptables *` is a root shell for the web user by another name.
`iptables --modprobe=/path/to/anything` runs that path as root, so a wildcard
grant on iptables escalates rather than restricts. I added that rule while
fixing a permissions gap, which is a worse outcome than the gap. It is gone,
and a test now fails on any trailing-wildcard grant to a tool that can execute
another program -- iptables, nft, tcpdump, find, awk, sed, perl, python, env.
The other finding: checking only the binary made the coverage test far weaker
than it looked. With `sysctl` present anywhere in the allow-list, deleting the
`net.ipv4.ip_forward=0` grant still passed -- and the portal would then be
unable to restore forwarding on teardown. Each required command is now matched
in full, and each is mutation-checked individually, including that exact
single-line case.
Scope pulled in deliberately. The first version of this test tried to assert
that *every* sudo call in the codebase is granted. Run honestly, it showed the
portal also runs iptables, nft, `ip addr`, `ip link` and `cp` with arguments
built at runtime -- an interface name, a port. Those cannot be granted safely
in a sudoers file: the rule needs a trailing wildcard, and that is the
escalation above. Closing that half needs a privileged helper that builds the
rules itself and takes only an interface and a port, granted the way
safe_plugin_rm.sh already is. That is a design decision, not a one-line grant,
so the test now pins the four commands this change actually grants and the
docstring says plainly what it does not cover.
Better a narrow test that is true than a broad one that is not.
(cherry picked from commit 500cfbc9f4)
* fix(install): pin PATH, keep unit order, and tighten the sudoers assertions
Three review findings, all correct.
The installer resolved binaries through an inherited PATH and wrote whatever
it found into sudoers as NOPASSWD grants. first_time_install.sh re-execs
itself with `sudo -E`, which preserves the caller's environment, so a writable
directory early in PATH turned a compromise of the low-privilege web user into
permanent root -- via a file the installer itself wrote. PATH is now pinned to
the system directories before anything is resolved, and every resolved binary
must be root-owned and unwritable by anyone else before it reaches the
sudoers file.
_unit_body() sorted a unit's lines before comparing. Order is not noise in a
systemd unit: repeated ExecStartPre=/ExecStartPost= run in the order they
appear, and a directive that moves between [Unit], [Service] and [Install]
means something different where it lands. The drift check reported no drift
for units that had genuinely changed. Order is preserved now.
Two of that check's own tests asserted the wrong thing --
test_reordered_directives_are_not_drift said so in its name -- and are
inverted, with a second covering a directive moved between sections. The
cosmetic-difference test now varies comments, blank lines and indentation,
which is what the installer actually drops, rather than reversing the file.
The sudoers assertions matched command prefixes, so
`sysctl -w net.ipv4.ip_forward=0 *` satisfied the requirement while granting
the caller arbitrary trailing arguments as root. They are exact now. The
wildcard check also normalises ${NFT_PATH} the same way as $NFT_PATH; the
brace is not a word boundary, so that spelling was skipped entirely.
Verified by reintroducing each: a widened required grant fails the exact
match, `${NFT_PATH} *` fails the wildcard check, and require_trusted_binary
refuses a non-root-owned, world-writable, or missing binary.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
311 lines
13 KiB
Python
311 lines
13 KiB
Python
"""
|
|
Startup Validator
|
|
|
|
Validates system configuration, plugins, and dependencies on startup.
|
|
Fails fast with clear error messages to prevent runtime issues.
|
|
"""
|
|
|
|
import os
|
|
from typing import Any, List, Optional, Tuple
|
|
from pathlib import Path
|
|
from src.exceptions import ConfigError, PluginError, CacheError
|
|
from src.logging_config import get_logger
|
|
|
|
|
|
class StartupValidator:
|
|
"""Validates system state on startup."""
|
|
|
|
def __init__(self, config_manager: Any, plugin_manager: Optional[Any] = None,
|
|
cache_manager: Optional[Any] = None) -> None:
|
|
"""
|
|
Initialize the startup validator.
|
|
|
|
Args:
|
|
config_manager: ConfigManager instance
|
|
plugin_manager: Optional PluginManager instance
|
|
cache_manager: The CacheManager the application will actually use.
|
|
Pass it. Without one this validator builds its own just to read
|
|
a directory path, which reports on a cache the app does not
|
|
use and leaves behind a cleanup thread that nothing stops --
|
|
validation runs twice per startup, so that was two of them.
|
|
"""
|
|
self.config_manager = config_manager
|
|
self.plugin_manager = plugin_manager
|
|
self.cache_manager = cache_manager
|
|
self.logger = get_logger(__name__)
|
|
self.errors: List[str] = []
|
|
self.warnings: List[str] = []
|
|
|
|
def validate_all(self) -> Tuple[bool, List[str], List[str]]:
|
|
"""
|
|
Run all validation checks.
|
|
|
|
Returns:
|
|
Tuple of (is_valid, errors, warnings)
|
|
"""
|
|
self.logger.info("Starting startup validation...")
|
|
|
|
# Fresh lists each run — without this, calling validate_all() twice
|
|
# duplicated every message.
|
|
self.errors = []
|
|
self.warnings = []
|
|
|
|
# Validate configuration
|
|
self._validate_config()
|
|
|
|
# Validate cache directory
|
|
self._validate_cache_directory()
|
|
|
|
# Validate display configuration
|
|
self._validate_display_config()
|
|
|
|
# Validate plugins if plugin manager is available
|
|
if self.plugin_manager:
|
|
self._validate_plugins()
|
|
|
|
# Warn when the running systemd unit no longer matches the repo's
|
|
self._validate_systemd_units()
|
|
|
|
is_valid = len(self.errors) == 0
|
|
|
|
if is_valid:
|
|
self.logger.info("Startup validation passed")
|
|
if self.warnings:
|
|
self.logger.warning(f"Startup validation completed with {len(self.warnings)} warning(s)")
|
|
else:
|
|
self.logger.error(f"Startup validation failed with {len(self.errors)} error(s)")
|
|
|
|
return (is_valid, self.errors.copy(), self.warnings.copy())
|
|
|
|
#: Units this project installs, and where each is installed to.
|
|
_UNITS = (
|
|
("systemd/ledmatrix.service", "/etc/systemd/system/ledmatrix.service"),
|
|
("systemd/ledmatrix-web.service", "/etc/systemd/system/ledmatrix-web.service"),
|
|
)
|
|
|
|
def _validate_systemd_units(self) -> None:
|
|
"""Warn when an installed unit has drifted from the repo's template.
|
|
|
|
Nothing re-applies these after the first install. `git pull` -- which is
|
|
what the web UI's update button runs -- brings a new template into the
|
|
checkout, but nothing copies it to /etc/systemd/system and nothing runs
|
|
`systemctl daemon-reload`, so the unit that actually runs is whatever
|
|
first_time_install.sh wrote on day one.
|
|
|
|
That makes every hardening added to a unit inert on existing installs.
|
|
Measured on one rig: the installed unit was thirteen days older than the
|
|
repo's and differed in content, so a MemoryMax the repo had specified
|
|
was not being enforced at all -- `systemctl show` reported
|
|
MemoryMax=infinity.
|
|
|
|
A warning rather than an error, and certainly 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.
|
|
The remedy is to re-run scripts/install/install_service.sh.
|
|
"""
|
|
try:
|
|
project_root = Path(__file__).resolve().parent.parent
|
|
for template_rel, installed_path in self._UNITS:
|
|
template = project_root / template_rel
|
|
installed = Path(installed_path)
|
|
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
|
|
|
|
if self._unit_body(expected) != self._unit_body(actual):
|
|
self.warnings.append(
|
|
f"{installed.name} differs from {template_rel}; the "
|
|
"installed unit is not refreshed by an update, so "
|
|
"settings added to the template are not in effect. "
|
|
"Re-run scripts/install/install_service.sh to apply them."
|
|
)
|
|
except OSError as e:
|
|
self.logger.debug("Could not compare systemd units: %s", e)
|
|
|
|
@staticmethod
|
|
def _unit_body(text: str) -> str:
|
|
"""A unit's meaningful lines, in order: no comments, no blanks.
|
|
|
|
Order is preserved deliberately. This used to sort, which made the
|
|
comparison insensitive to two changes that matter in a systemd unit:
|
|
repeated directives such as ExecStartPre= and 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. A drift check that normalises those away reports no
|
|
drift for a unit that has genuinely changed.
|
|
"""
|
|
lines = []
|
|
for line in text.splitlines():
|
|
line = line.strip()
|
|
if line and not line.startswith("#"):
|
|
lines.append(line)
|
|
return "\n".join(lines)
|
|
|
|
def _validate_config(self) -> None:
|
|
"""Validate configuration files."""
|
|
try:
|
|
config = self.config_manager.load_config()
|
|
|
|
# Check for required top-level keys
|
|
required_keys = ['display', 'timezone']
|
|
for key in required_keys:
|
|
if key not in config:
|
|
self.errors.append(f"Missing required configuration key: {key}")
|
|
|
|
# Validate display configuration
|
|
display_config = config.get('display', {})
|
|
if not display_config:
|
|
self.errors.append("Display configuration is missing or empty")
|
|
|
|
except ConfigError as e:
|
|
self.errors.append(f"Configuration error: {e}")
|
|
except Exception as e:
|
|
self.errors.append(f"Unexpected error validating configuration: {e}")
|
|
|
|
def _validate_cache_directory(self) -> None:
|
|
"""Validate cache directory permissions."""
|
|
try:
|
|
cache_manager = self.cache_manager
|
|
if cache_manager is None:
|
|
# No caller supplied one (older embedders, direct use in a
|
|
# script). Build one, but do not leave its cleanup thread
|
|
# running behind us -- this instance is discarded on the next
|
|
# line but the thread is a closure over it, so it would never
|
|
# be collected.
|
|
from src.cache_manager import CacheManager
|
|
cache_manager = CacheManager()
|
|
try:
|
|
cache_dir = cache_manager.get_cache_dir()
|
|
finally:
|
|
cache_manager.stop_cleanup_thread()
|
|
else:
|
|
cache_dir = cache_manager.get_cache_dir()
|
|
|
|
if not cache_dir:
|
|
self.warnings.append("Cache directory not available - caching will be disabled")
|
|
return
|
|
|
|
# Check if directory exists and is writable
|
|
if not os.path.exists(cache_dir):
|
|
self.errors.append(f"Cache directory does not exist: {cache_dir}")
|
|
return
|
|
|
|
if not os.access(cache_dir, os.W_OK):
|
|
self.errors.append(f"Cache directory is not writable: {cache_dir}")
|
|
return
|
|
|
|
# Test write access
|
|
test_file = os.path.join(cache_dir, '.startup_test')
|
|
try:
|
|
with open(test_file, 'w') as f:
|
|
f.write('test')
|
|
os.remove(test_file)
|
|
except (IOError, OSError) as e:
|
|
self.errors.append(f"Cannot write to cache directory {cache_dir}: {e}")
|
|
|
|
except Exception as e:
|
|
self.warnings.append(f"Could not validate cache directory: {e}")
|
|
|
|
def _validate_display_config(self) -> None:
|
|
"""Validate display configuration."""
|
|
try:
|
|
config = self.config_manager.get_config()
|
|
display_config = config.get('display', {})
|
|
|
|
if not display_config:
|
|
self.errors.append("Display configuration is missing")
|
|
return
|
|
|
|
hardware_config = display_config.get('hardware', {})
|
|
if not hardware_config:
|
|
self.errors.append("Display hardware configuration is missing")
|
|
return
|
|
|
|
# Check required hardware settings
|
|
required_hardware = ['rows', 'cols']
|
|
for key in required_hardware:
|
|
if key not in hardware_config:
|
|
self.warnings.append(f"Display hardware setting '{key}' not specified, using default")
|
|
|
|
except Exception as e:
|
|
self.warnings.append(f"Could not validate display configuration: {e}")
|
|
|
|
def _validate_plugins(self) -> None:
|
|
"""Validate plugin configurations and dependencies."""
|
|
if not self.plugin_manager:
|
|
return
|
|
|
|
try:
|
|
# Get enabled plugins from config
|
|
config = self.config_manager.get_config()
|
|
discovered_plugins = self.plugin_manager.discover_plugins()
|
|
|
|
# Check for enabled plugins that don't exist
|
|
for plugin_id, plugin_config in config.items():
|
|
# Skip non-plugin config sections
|
|
if plugin_id in ['display', 'schedule', 'timezone', 'plugin_system']:
|
|
continue
|
|
|
|
if not isinstance(plugin_config, dict):
|
|
continue
|
|
|
|
if plugin_config.get('enabled', False):
|
|
if plugin_id not in discovered_plugins:
|
|
self.warnings.append(f"Plugin '{plugin_id}' is enabled but not found in plugins directory")
|
|
|
|
# Validate plugin configurations
|
|
for plugin_id in discovered_plugins:
|
|
plugin_config = config.get(plugin_id, {})
|
|
if plugin_config.get('enabled', False):
|
|
# Check if plugin can be loaded (without actually loading it)
|
|
plugin_dir = self.plugin_manager.get_plugin_directory(plugin_id)
|
|
if plugin_dir:
|
|
manifest_path = Path(plugin_dir) / "manifest.json"
|
|
if not manifest_path.exists():
|
|
self.errors.append(f"Plugin '{plugin_id}' manifest.json not found")
|
|
|
|
except Exception as e:
|
|
self.warnings.append(f"Could not validate plugins: {e}")
|
|
|
|
def raise_on_errors(self) -> None:
|
|
"""
|
|
Raise exceptions if validation errors exist.
|
|
|
|
Raises:
|
|
ConfigError: If configuration validation fails
|
|
CacheError: If cache validation fails
|
|
PluginError: If plugin validation fails
|
|
"""
|
|
if not self.errors:
|
|
return
|
|
|
|
# Group errors by type
|
|
config_errors = [e for e in self.errors if 'configuration' in e.lower() or 'config' in e.lower()]
|
|
cache_errors = [e for e in self.errors if 'cache' in e.lower()]
|
|
plugin_errors = [e for e in self.errors if 'plugin' in e.lower()]
|
|
other_errors = [e for e in self.errors if e not in config_errors + cache_errors + plugin_errors]
|
|
|
|
# Raise appropriate exceptions
|
|
if config_errors:
|
|
raise ConfigError("Configuration validation failed", context={'errors': config_errors})
|
|
|
|
if cache_errors:
|
|
raise CacheError("Cache validation failed", context={'errors': cache_errors})
|
|
|
|
if plugin_errors:
|
|
raise PluginError("Plugin validation failed", context={'errors': plugin_errors})
|
|
|
|
if other_errors:
|
|
raise ConfigError("Startup validation failed", context={'errors': other_errors})
|
|
|