diff --git a/docs/LOW_MEMORY_BOARDS.md b/docs/LOW_MEMORY_BOARDS.md index 98741f2b..186e8a4a 100644 --- a/docs/LOW_MEMORY_BOARDS.md +++ b/docs/LOW_MEMORY_BOARDS.md @@ -57,7 +57,10 @@ grep memory /sys/fs/cgroup/cgroup.controllers ``` If that prints nothing, add `cgroup_enable=memory cgroup_memory=1` to the -single line in `/boot/firmware/cmdline.txt` and reboot. +kernel command line and reboot. Edit whichever file your image uses — +`/boot/firmware/cmdline.txt` on current Raspberry Pi OS, `/boot/cmdline.txt` on +older layouts (the installer checks the first and falls back to the second). +Everything must stay on a single line. This changes the failure mode from "the board becomes unreachable" to "the display service restarts". It is a safety net, not a fix. @@ -76,6 +79,14 @@ plugins that poll infrequently. Environment=LEDMATRIX_CACHE_MAX_ENTRIES=75 ``` +Writing the file does not change the running service. Reload systemd and +restart it: + +```bash +sudo systemctl daemon-reload +sudo systemctl restart ledmatrix +``` + Fewer entries means more API calls, so lower this only while you are actually short of memory. diff --git a/docs/SSH_UNAVAILABLE_AFTER_INSTALL.md b/docs/SSH_UNAVAILABLE_AFTER_INSTALL.md index 82f98a70..bc065eb1 100644 --- a/docs/SSH_UNAVAILABLE_AFTER_INSTALL.md +++ b/docs/SSH_UNAVAILABLE_AFTER_INSTALL.md @@ -26,7 +26,7 @@ On a 512MB or 1GB board, memory exhaustion stops `sshd` being able to fork a session process. The connection is accepted and then closed immediately, before any banner: -``` +```text kex_exchange_identification: Connection closed by remote host ``` @@ -205,11 +205,23 @@ The web interface allows you to: ## Summary -**SSH becomes unavailable because**: +**SSH becomes unavailable because** — two unrelated causes, and they need +different responses: + +*AP mode (most common):* - WiFi monitor service enables AP mode when WiFi disconnects - AP mode switches WiFi from client to access point mode - Pi loses connection to your original network +*Memory exhaustion (low-memory boards):* +- The board runs out of memory, so `sshd` cannot fork a session process +- The connection is accepted and closed before any banner +- Ping still answers and the web UI still responds, so it looks healthy +- The panel is usually dark and the service cannot restart +- **Only a power cycle clears this** — there is no remote recovery, because + every remote route needs a new process +- Prevention and tuning: [LOW_MEMORY_BOARDS.md](LOW_MEMORY_BOARDS.md) + **To regain SSH**: 1. Connect to **LEDMatrix-Setup** AP network (password: `ledmatrix123`) 2. SSH to `192.168.4.1` diff --git a/first_time_install.sh b/first_time_install.sh index b3cb190b..471f0516 100644 --- a/first_time_install.sh +++ b/first_time_install.sh @@ -1694,12 +1694,24 @@ fi # a runaway takes the whole board down (sshd can no longer fork, the panel goes # dark) rather than just restarting the one service. if [ "$SKIP_PERF" != "1" ] && [ -f "$CMDLINE_FILE" ]; then - if grep -q 'cgroup_enable=memory' "$CMDLINE_FILE"; then - echo "cgroup_enable=memory already present in $CMDLINE_FILE" + # Both parameters are required for the memory controller, and they can get + # separated -- an image, another tool or a half-applied earlier run can + # leave one without the other. Checking only cgroup_enable=memory would + # report success while MemoryMax= silently does nothing, so each is checked + # and appended independently. + cgroup_missing="" + for cgroup_param in cgroup_enable=memory cgroup_memory=1; do + if ! grep -qw "$cgroup_param" "$CMDLINE_FILE"; then + cgroup_missing="$cgroup_missing $cgroup_param" + fi + done + if [ -z "$cgroup_missing" ]; then + echo "cgroup memory parameters already present in $CMDLINE_FILE" else - echo "Adding cgroup_enable=memory to $CMDLINE_FILE..." + echo "Adding${cgroup_missing} to $CMDLINE_FILE..." cp "$CMDLINE_FILE" "$CMDLINE_FILE.bak" 2>/dev/null || true - sed -i '1 s/$/ cgroup_enable=memory cgroup_memory=1/' "$CMDLINE_FILE" + # The kernel command line must stay on one line. + sed -i "1 s|\$|${cgroup_missing}|" "$CMDLINE_FILE" echo " Takes effect after reboot. Verify with:" echo " grep memory /sys/fs/cgroup/cgroup.controllers" fi @@ -1709,17 +1721,41 @@ fi # These images default to volatile storage: journald keeps everything in /run # (tmpfs), so every reboot destroys the logs — including the ones that would # explain why the board rebooted. Capped so an SD card is not worn out by logs. -if [ -d /var/log/journal ] && [ -n "$(ls -A /var/log/journal 2>/dev/null)" ]; then - echo "Persistent journald storage already enabled" +# A non-empty /var/log/journal does not prove journald is configured the way +# this needs: the directory survives a switch back to volatile storage, and it +# says nothing about whether a size cap is set. Read the effective +# configuration instead, and only write the keys the user has not set +# themselves so an explicit local limit is preserved. +journald_effective() { + # systemd-analyze merges journald.conf with every drop-in; grep is the + # fallback for images that ship without it. + if command -v systemd-analyze >/dev/null 2>&1 && + systemd-analyze cat-config systemd/journald.conf >/dev/null 2>&1; then + systemd-analyze cat-config systemd/journald.conf 2>/dev/null + else + cat /etc/systemd/journald.conf /etc/systemd/journald.conf.d/*.conf 2>/dev/null + fi +} +journald_conf="$(journald_effective)" +journald_storage="$(printf '%s\n' "$journald_conf" | grep -E '^[[:space:]]*Storage=' | tail -n1 | cut -d= -f2 | tr -d '[:space:]')" +journald_cap="$(printf '%s\n' "$journald_conf" | grep -E '^[[:space:]]*SystemMaxUse=' | tail -n1 | cut -d= -f2 | tr -d '[:space:]')" + +if [ "$journald_storage" = "persistent" ] && [ -n "$journald_cap" ]; then + echo "Persistent journald storage already configured (SystemMaxUse=$journald_cap)" else echo "Enabling persistent journald storage..." mkdir -p /etc/systemd/journald.conf.d - cat > /etc/systemd/journald.conf.d/ledmatrix-persistent.conf <<'JOURNALD' -# Installed by LEDMatrix first_time_install.sh -[Journal] -Storage=persistent -SystemMaxUse=64M -JOURNALD + { + echo "# Installed by LEDMatrix first_time_install.sh" + echo "[Journal]" + echo "Storage=persistent" + if [ -n "$journald_cap" ]; then + echo "# SystemMaxUse left to your existing setting ($journald_cap)" + else + # Capped so logs cannot wear out or fill an SD card. + echo "SystemMaxUse=64M" + fi + } > /etc/systemd/journald.conf.d/ledmatrix-persistent.conf mkdir -p /var/log/journal systemd-tmpfiles --create --prefix /var/log/journal >/dev/null 2>&1 || true systemctl restart systemd-journald >/dev/null 2>&1 || true diff --git a/src/cache/memory_cache.py b/src/cache/memory_cache.py index 60993427..55ca010d 100644 --- a/src/cache/memory_cache.py +++ b/src/cache/memory_cache.py @@ -134,6 +134,32 @@ class MemoryCache: with self._lock: self._cache[key] = value self._timestamps[key] = time.time() + # Enforce the ceiling here rather than leaving it to the periodic + # cleanup, which only runs every cleanup_interval seconds (300 by + # default). A burst of inserts between two sweeps could otherwise + # take the cache far past _max_size, which is the memory growth this + # limit exists to prevent -- and on a 1GB board that is the + # difference between a bounded cache and an unreachable Pi. + self._evict_over_limit_locked() + + def _evict_over_limit_locked(self) -> int: + """Drop oldest entries until the cache is within _max_size. + + Caller must hold self._lock. Returns the number of entries removed. + """ + excess = len(self._cache) - self._max_size + if excess <= 0: + return 0 + oldest = sorted( + self._timestamps.items(), + key=lambda item: float(item[1]) if isinstance(item[1], (int, float)) else 0.0 + ) + removed = 0 + for key, _ in oldest[:excess]: + self._cache.pop(key, None) + self._timestamps.pop(key, None) + removed += 1 + return removed def clear(self, key: Optional[str] = None) -> None: """ @@ -190,22 +216,8 @@ class MemoryCache: self._timestamps.pop(key, None) removed_count += 1 - # Enforce size limit by removing oldest entries if cache is too large - if len(self._cache) > self._max_size: - # Sort by timestamp (oldest first) - sorted_entries = sorted( - self._timestamps.items(), - key=lambda x: float(x[1]) if isinstance(x[1], (int, float)) else 0 - ) - - # Remove oldest entries until we're under the limit - excess_count = len(self._cache) - self._max_size - for i in range(excess_count): - if i < len(sorted_entries): - key = sorted_entries[i][0] - self._cache.pop(key, None) - self._timestamps.pop(key, None) - removed_count += 1 + # Same ceiling enforcement set() uses, so the two cannot drift. + removed_count += self._evict_over_limit_locked() self._last_cleanup = current_time diff --git a/src/plugin_system/plugin_health.py b/src/plugin_system/plugin_health.py index 71a05ea3..39b5be3e 100644 --- a/src/plugin_system/plugin_health.py +++ b/src/plugin_system/plugin_health.py @@ -7,7 +7,7 @@ and circuit breaker state. Provides automatic recovery mechanisms. import time import logging -from typing import Dict, Optional, Any +from typing import Dict, Optional, Any, Tuple from enum import Enum @@ -65,20 +65,47 @@ class PluginHealthTracker: ) if isinstance(cached, dict) and cached: - return cached + # Complete it rather than trusting it: a persisted record can be + # missing fields the callers index directly (a partial write, a + # restored backup, an older schema), and returning it verbatim makes + # record_success / record_failure raise KeyError, which takes the + # display down in a restart loop that survives reboots because the + # bad entry is on disk. + state, repaired = self._repair_health_state(cached) + if repaired: + self.logger.warning( + f"Repaired health state for {plugin_id}: " + f"{sorted(repaired)} missing or invalid, using defaults for those." + ) + return state - # A cache entry that is not a dict means the persisted state was written - # by something other than _save_health_state (a key collision, a partial - # write, a restored backup). Returning it verbatim makes every caller - # blow up on .get(), which takes the display down in a restart loop that - # survives reboots because the bad entry is on disk. Discard and rebuild. + # Not a dict at all: written by something other than + # _save_health_state (a key collision, a corrupted entry). Nothing to + # salvage. if cached is not None and not isinstance(cached, dict): self.logger.warning( f"Discarding malformed health state for {plugin_id}: expected " f"dict, got {type(cached).__name__}. Falling back to defaults." ) - - # Default state + + return self._default_health_state() + + def _save_health_state(self, plugin_id: str, state: Dict[str, Any]) -> None: + """Save health state to cache.""" + cache_key = self._get_health_key(plugin_id) + self.cache_manager.set(cache_key, state) # Persist indefinitely + self._health_state[plugin_id] = state + + # The fields callers index directly (state['circuit_state'] and friends). + # A cached dict missing any of them raises KeyError deep in record_success / + # record_failure, so the value is completed before it is handed out. + _COUNTER_FIELDS = ('consecutive_failures', 'total_failures', 'total_successes') + _TIMESTAMP_FIELDS = ('last_success_time', 'last_failure_time', + 'circuit_opened_time', 'half_open_start_time') + + @staticmethod + def _default_health_state() -> Dict[str, Any]: + """A fresh state with every field the callers expect.""" return { 'consecutive_failures': 0, 'total_failures': 0, @@ -88,15 +115,46 @@ class PluginHealthTracker: 'circuit_state': CircuitState.CLOSED.value, 'circuit_opened_time': None, 'half_open_start_time': None, - 'last_error': None + 'last_error': None, } - - def _save_health_state(self, plugin_id: str, state: Dict[str, Any]) -> None: - """Save health state to cache.""" - cache_key = self._get_health_key(plugin_id) - self.cache_manager.set(cache_key, state) # Persist indefinitely - self._health_state[plugin_id] = state - + + @classmethod + def _repair_health_state(cls, cached: Dict[str, Any]) -> Tuple[Dict[str, Any], list]: + """Return `cached` completed against the defaults, plus what was repaired. + + Per-field rather than all-or-nothing: a record that has real failure + counts but is missing `last_error` should keep the counts, not be reset + to healthy. Only values that are absent or the wrong type fall back to + the default, so a partial or older-schema record survives with whatever + it does carry, while every field the callers index is guaranteed present + and of a usable type. + """ + state = cls._default_health_state() + repaired = [] + for field, default in state.items(): + if field not in cached: + repaired.append(field) + continue + value = cached[field] + if field in cls._COUNTER_FIELDS: + ok = isinstance(value, int) and not isinstance(value, bool) and value >= 0 + elif field in cls._TIMESTAMP_FIELDS: + ok = value is None or isinstance(value, (int, float)) + elif field == 'circuit_state': + ok = value in {member.value for member in CircuitState} + else: # last_error + ok = value is None or isinstance(value, str) + if ok: + state[field] = value + else: + repaired.append(field) + # Anything the schema has since grown (degraded, degraded_reason) is + # read with .get() by its callers, so carry it through untouched. + for field, value in cached.items(): + if field not in state: + state[field] = value + return state, repaired + def get_health_state(self, plugin_id: str, force_reload: bool = False) -> Dict[str, Any]: """Get current health state for a plugin. diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index e2e2d11c..d8ae0e3d 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -75,14 +75,30 @@ def _extra_dependencies(dist_name: str, extras) -> Optional[List[Requirement]]: return gated -def _extras_are_satisfied(req: Requirement) -> bool: +def _extras_are_satisfied(req: Requirement, _visited: Optional[set] = None) -> bool: """Check the dependencies pulled in by req's extras are installed. - One level deep, not transitive: enough to tell "the extra was installed" - from "the extra was never installed", which is all the caller needs to - decide whether pip has work to do. Anything unreadable returns False, so - the caller still falls through to pip. + Follows extras through nested extras. A gated dependency can itself request + one (`requests[socks]`), and checking only that `requests` is installed at + an acceptable version says nothing about whether the socks extra's own + dependency is there -- so the caller would skip pip and the plugin would + fail at import instead. Plain dependencies are still checked one level + deep, which is all that is needed to tell "the extra was installed" from + "the extra was never installed". + + `_visited` carries the (distribution, extras) pairs already seen, so a + dependency cycle between extras terminates instead of recursing forever. + Anything unreadable returns False, so the caller still falls through to pip. """ + if _visited is None: + _visited = set() + marker = (req.name.lower(), frozenset(e.lower() for e in req.extras)) + if marker in _visited: + # Already accounted for higher up the chain; treating a cycle as + # satisfied here is safe because the outer frame still has to pass. + return True + _visited.add(marker) + gated = _extra_dependencies(req.name, req.extras) if gated is None: return False @@ -94,6 +110,8 @@ def _extras_are_satisfied(req: Requirement) -> bool: return False if dep.specifier and not dep.specifier.contains(dep_version, prereleases=True): return False + if dep.extras and not _extras_are_satisfied(dep, _visited): + return False return True diff --git a/test/test_cache_manager.py b/test/test_cache_manager.py index 8a93a39d..9d1195a8 100644 --- a/test/test_cache_manager.py +++ b/test/test_cache_manager.py @@ -458,3 +458,26 @@ class TestDiskCacheWriteEconomy: cache = DiskCache(cache_dir=str(tmp_path)) cache.set("k", {"when": datetime(2026, 7, 12, 10, 30)}) assert cache.get("k") == {"when": "2026-07-12T10:30:00"} + + +# --- the ceiling has to hold between cleanup sweeps --------------------------- + +def test_memory_cache_enforces_ceiling_on_every_write(): + """_cleanup_memory_cache only runs every cleanup_interval seconds (300 by + default). If set() accepted entries without bound in between, a burst could + take the cache far past max_size -- which is the unbounded growth the limit + exists to prevent, and on a 1GB board the difference between a bounded cache + and a Pi that cannot fork. + """ + from src.cache.memory_cache import MemoryCache + + cache = MemoryCache(max_size=150, cleanup_interval=300.0) + for i in range(1000): + cache.set(f"k{i}", {"v": i}) + + assert len(cache._cache) <= 150 + # The timestamp map has to be evicted alongside the values, or it becomes + # the leak instead. + assert len(cache._timestamps) <= 150 + assert cache.get("k999") is not None, "the newest write must survive" + assert cache.get("k0") is None, "the oldest must be the one evicted" diff --git a/test/test_plugin_health.py b/test/test_plugin_health.py index fd647a4b..4b1a7636 100644 --- a/test/test_plugin_health.py +++ b/test/test_plugin_health.py @@ -91,3 +91,77 @@ def test_force_reload_refreshes_stale_in_memory_snapshot(): # and it asked the cache to bypass the in-memory tier (memory_ttl=0). assert any(c.kwargs.get("memory_ttl") == 0 for c in cache.get.call_args_list) + + +# --- persisted state that does not match the current schema ------------------- +# +# A record on disk can be missing fields the callers index directly: a partial +# write, a restored backup, or a state written by an older schema. Returning it +# verbatim raises KeyError inside record_success / record_failure, which takes +# the display down in a restart loop that survives reboots, because the bad +# entry is on disk and gets read again on the way back up. Observed in the wild +# as `plugin clock-simple operation failed: 'circuit_state'`, repeating ~50x a +# minute with the panel frozen. + +_INDEXED_FIELDS = ( + "consecutive_failures", "total_failures", "total_successes", + "last_success_time", "last_failure_time", "circuit_state", + "circuit_opened_time", "half_open_start_time", "last_error", +) + + +def _tracker_reading(persisted): + cache = _cache() + cache.get.return_value = persisted + return PluginHealthTracker(cache) + + +def test_partial_state_is_completed_not_returned_raw(): + """The shape seen in the wild: one field, everything else absent.""" + state = _tracker_reading({"circuit_state": "closed"}).get_health_state("p") + for field in _INDEXED_FIELDS: + assert field in state, f"{field} missing; callers index it directly" + + +def test_repair_keeps_real_failure_history(): + """A record with genuine counts must not be reset to healthy just because + an optional field is absent -- that would clear a tripped breaker.""" + state = _tracker_reading({ + "consecutive_failures": 5, + "total_failures": 5, + "circuit_state": "open", + }).get_health_state("p") + assert state["consecutive_failures"] == 5 + assert state["total_failures"] == 5 + assert state["circuit_state"] == "open" + + +def test_wrong_types_fall_back_per_field(): + """A counter persisted as a string would pass a membership check and then + fail on the first += 1; an unknown circuit_state would take a branch the + breaker has no handling for.""" + state = _tracker_reading({ + "consecutive_failures": "3", + "circuit_state": "melted", + "total_failures": 7, + }).get_health_state("p") + assert state["consecutive_failures"] == 0 + assert state["circuit_state"] == CircuitState.CLOSED.value + assert state["total_failures"] == 7, "valid neighbours must survive" + + +def test_newer_fields_are_carried_through(): + """degraded/degraded_reason are read with .get() and are not part of the + indexed set; repairing must not drop them.""" + state = _tracker_reading({ + "circuit_state": "closed", "degraded": True, "degraded_reason": "x", + }).get_health_state("p") + assert state["degraded"] is True + assert state["degraded_reason"] == "x" + + +def test_recording_against_a_repaired_state_does_not_raise(): + """The actual failure: record_failure indexing a field that was not there.""" + tracker = _tracker_reading({"circuit_state": "closed"}) + tracker.record_failure("p", Exception("boom")) + tracker.record_success("p")