fix: address review findings on the low-memory work

Nine CodeRabbit findings, five in code.

**Health state (the one that matters).** The non-dict guard did not cover a
dict missing fields the callers index directly, which is the shape actually
seen in the wild: a record carrying only circuit_state produced
`plugin clock-simple operation failed: 'circuit_state'` about fifty times a
minute with the panel frozen. The record is now completed against the
defaults per field rather than trusted or discarded wholesale. Per field
matters: a first pass rejected any incomplete record outright, which reset a
tripped breaker and real failure counts to healthy because one optional
field was absent -- an existing test caught it. Values of the wrong type
(a counter persisted as a string, an unknown circuit_state) fall back
individually, valid neighbours survive, and newer fields the schema has
grown since (degraded, degraded_reason) are carried through untouched.

**Cache ceiling.** MemoryCache.set() accepted entries without bound between
cleanup sweeps, which run every 300s by default, so a burst could take the
cache far past max_size -- the unbounded growth the limit exists to stop.
Eviction now runs under the same lock on every write, sharing one helper
with the periodic sweep so the two cannot drift.

**Installer, cgroups.** Only cgroup_enable=memory was checked, so a board
carrying that without cgroup_memory=1 reported success and got no change,
leaving MemoryMax= inert. Each parameter is now checked and appended
independently; verified against all four combinations, single line preserved.

**Installer, journald.** Persistence was inferred from /var/log/journal being
non-empty, which proves neither Storage=persistent nor a size cap -- the
directory survives a switch back to volatile. The effective configuration is
read instead (systemd-analyze cat-config, falling back to the conf files),
and an explicitly configured SystemMaxUse is preserved rather than
overwritten. Verified across volatile, persistent-without-cap,
persistent-with-user-cap, cap-without-storage, and commented-only configs.

**Dependency extras.** _extras_are_satisfied stopped at one level, so a
gated dependency that itself requests an extra (requests[socks]) passed on
the base distribution's version while the extra's own dependency was
missing, and pip was skipped. It now recurses, with a visited
(distribution, extras) set so a cycle terminates.

Docs: both kernel command-line paths documented (the installer falls back to
/boot/cmdline.txt), daemon-reload and restart added after the systemd
override example, memory exhaustion added to the SSH summary with its
power-cycle-only recovery, and a language on the fenced block for MD040.

Tests: five for the health-state repair including the exact wild shape and
that record_failure/record_success no longer raise against it, and one for
the cache ceiling. Both mutation-checked. Full suite 2927 passed, with the
one pre-existing tmpfs failure that also fails on main.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

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.
 

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`

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

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
             

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.
 

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
 
 

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"

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")