fix(plugins): one bad metrics cache entry should not stop every plugin

Caught live on a rig: every plugin failing, once each, continuously.

    ERROR - src.plugin_system.plugin_manager - plugin geochron operation failed:
    ResourceMetrics.__init__() got an unexpected keyword argument
    'consecutive_failures'

    ERROR - ... plugin text-display operation failed: ...
    ERROR - ... plugin news operation failed: ...
    ERROR - ... plugin odds-ticker operation failed: ...

with /api/v3/health reporting plugin_system: not_initialized while the display
process itself kept running and updating the panel.

`consecutive_failures` is a plugin_health field, not a metrics one.
get_metrics() does ResourceMetrics(**cached), which raises TypeError on a
single unrecognised key, and that exception escapes into plugin_manager and is
reported per plugin. One malformed cache entry takes the whole plugin system
down.

How a health-shaped record came to sit under a plugin_metrics key on that
machine is not established, and I could not finish the diagnosis: the rig went
back into its EIO failure mode partway through -- SSH resetting pre-banner,
systemctl unexecutable -- while the web API kept answering from RAM. Checked
before that: the cache files on disk are correctly shaped and separate, and
CacheManager.get() returns the right record for each key, so it is not a live
key collision. A restored backup mixing two machines' caches is the likeliest
explanation, and that rig had one restored onto it.

Either way the loader should not be brittle enough for the answer to matter.
plugin_health already repairs its records field by field rather than trusting
what is on disk; this does the same. Known fields are kept, unknown ones are
dropped and named once in the log so a genuine schema change stays visible
rather than being silently discarded, and a non-mapping entry no longer raises.

Keeping the known fields matters: discarding the record wholesale would throw
away real call counts and timings because of an unrelated stray key.

Mutation-checked: restoring ResourceMetrics(**cached) fails 6 checks, dropping
the whole record fails the field-preservation check, and dropping unknown
fields silently fails the logging check. 28 tests pass across the resource
monitor and plugin health suites.

src/backup_manager.py

@@ -111,12 +111,6 @@ class RestoreOptions:
     """Which sections of a backup should be restored."""
 
     restore_config: bool = True
-    #: Whether to take the backup's display.hardware block as well.
-    #: Off by default: that block describes the panel physically wired to
-    #: *this* device -- its size, chain length, mapping, multiplexing and
-    #: refresh cap. A backup carries the panel of the machine it was taken
-    #: on, and restoring one onto a different rig drives the wrong geometry.
-    restore_hardware: bool = False
     restore_secrets: bool = True
     restore_wifi: bool = True
     restore_fonts: bool = True
@@ -555,72 +549,6 @@ def _copy_file(src: Path, dst: Path) -> None:
         raise
 
 
-_HARDWARE_PATH = ("display", "hardware")
-
-
-def _restore_config_preserving_hardware(src: Path, dst: Path, keep_hardware: bool) -> None:
-    """Copy a backed-up config.json, optionally keeping the local panel block.
-
-    display.hardware describes the panel physically attached to this device:
-    cols, rows, chain_length, hardware_mapping, panel_type, multiplexing and
-    the refresh-rate cap. None of that travels with a configuration -- it is a
-    property of the machine. Restoring a backup taken on a 512x64 rig onto a
-    128x32 one used to overwrite the smaller panel's geometry with the larger
-    one's, which is not a setting the user can see going wrong; the display
-    simply stops being right.
-
-    Falls back to a plain copy when either file cannot be parsed, so a restore
-    never fails because of this.
-    """
-    if not keep_hardware:
-        _copy_file(src, dst)
-        return
-    try:
-        incoming = json.loads(src.read_text(encoding="utf-8"))
-        local = json.loads(dst.read_text(encoding="utf-8")) if dst.exists() else {}
-    except (OSError, ValueError) as exc:
-        logger.warning(
-            "[Backup] Could not merge local panel config (%s); restoring the "
-            "backup's config.json as-is", exc)
-        _copy_file(src, dst)
-        return
-
-    section, key = _HARDWARE_PATH
-    local_hw = (local.get(section) or {}).get(key)
-    if not isinstance(local_hw, dict) or not local_hw:
-        _copy_file(src, dst)
-        return
-
-    if not isinstance(incoming.get(section), dict):
-        incoming[section] = {}
-    incoming_hw = incoming[section].get(key)
-    incoming[section][key] = local_hw
-    if isinstance(incoming_hw, dict) and incoming_hw != local_hw:
-        logger.info(
-            "[Backup] Kept this device's display.hardware; the backup's panel "
-            "was %sx%s chain %s, this one is %sx%s chain %s",
-            incoming_hw.get("cols"), incoming_hw.get("rows"),
-            incoming_hw.get("chain_length"),
-            local_hw.get("cols"), local_hw.get("rows"),
-            local_hw.get("chain_length"))
-
-    # Write the merged result to a scratch file and hand it to _copy_file
-    # rather than renaming it into place here. _copy_file preserves the
-    # destination's mode and owner on purpose -- these config files are
-    # installed root-owned while the web interface that runs a restore is not
-    # root -- and a bare write would have replaced a root-owned config.json
-    # with one owned by the web user at whatever the umask allows.
-    scratch = dst.with_suffix(dst.suffix + ".merged-tmp")
-    try:
-        scratch.write_text(json.dumps(incoming, indent=2) + "\n", encoding="utf-8")
-        _copy_file(scratch, dst)
-    finally:
-        try:
-            scratch.unlink()
-        except OSError:
-            pass
-
-
 def restore_backup(
     zip_path: Path,
     project_root: Path,
@@ -656,9 +584,7 @@ def restore_backup(
         # Main config.
         if options.restore_config and (tmp_dir / _CONFIG_REL).exists():
             try:
-                _restore_config_preserving_hardware(
+                _copy_file(tmp_dir / _CONFIG_REL, project_root / _CONFIG_REL)
-                    tmp_dir / _CONFIG_REL, project_root / _CONFIG_REL,
-                    keep_hardware=not options.restore_hardware)
                 result.restored.append("config")
             except OSError as e:
                 logger.error("[Backup] Failed to restore config.json: %s", e, exc_info=True)

src/plugin_system/resource_monitor.py

@@ -9,7 +9,7 @@ import time
 import logging
 import threading
 from typing import Dict, Optional, Any, Callable
-from dataclasses import dataclass, field
+from dataclasses import dataclass, field, fields
 
 try:
     import psutil
@@ -102,6 +102,50 @@ class PluginResourceMonitor:
                 "psutil not available - resource monitoring will be limited to execution time only"
             )
     
+    def _metrics_from_cache(self, plugin_id: str, cached: Any) -> "ResourceMetrics":
+        """Build metrics from a cached record, ignoring anything unrecognised.
+
+        ResourceMetrics(**cached) raises TypeError on a single unexpected key,
+        and that exception escapes into plugin_manager, which reports it as
+        "plugin <id> operation failed". Every plugin fails, and the plugin
+        system never finishes initialising.
+
+        Seen on a live rig: every plugin failing with
+
+            ResourceMetrics.__init__() got an unexpected keyword argument
+            'consecutive_failures'
+
+        which is a plugin_health field, not a metrics one. How a health-shaped
+        record came to sit under a plugin_metrics key on that machine is not
+        established -- a restored backup that mixed two machines' caches is the
+        likeliest explanation -- but the loader should not be brittle enough for
+        it to matter. plugin_health already repairs its records field by field
+        rather than trusting whatever is on disk; this does the same.
+
+        Unknown keys are dropped and named once, so a genuine schema change is
+        visible in the log instead of silently discarded.
+        """
+        if not isinstance(cached, dict):
+            self.logger.warning(
+                "Ignoring cached metrics for %s: expected a mapping, got %s",
+                plugin_id, type(cached).__name__)
+            return ResourceMetrics()
+
+        known = {f.name for f in fields(ResourceMetrics)}
+        unknown = sorted(set(cached) - known)
+        if unknown:
+            self.logger.warning(
+                "Dropping unrecognised field(s) from cached metrics for %s: %s",
+                plugin_id, ", ".join(unknown))
+        usable = {k: v for k, v in cached.items() if k in known}
+        try:
+            return ResourceMetrics(**usable)
+        except (TypeError, ValueError) as e:
+            self.logger.warning(
+                "Cached metrics for %s unusable (%s); starting fresh",
+                plugin_id, e)
+            return ResourceMetrics()
+
     def _get_metrics_key(self, plugin_id: str) -> str:
         """Get cache key for plugin metrics."""
         return f"plugin_metrics:{plugin_id}"
@@ -126,7 +170,7 @@ class PluginResourceMonitor:
                     cache_key, max_age=None, memory_ttl=0 if force_reload else None
                 )
                 if cached:
-                    metrics = ResourceMetrics(**cached)
+                    metrics = self._metrics_from_cache(plugin_id, cached)
                 else:
                     metrics = ResourceMetrics()
                 self._metrics[plugin_id] = metrics

test/test_metrics_cache_unknown_fields.py

@@ -0,0 +1,100 @@
+"""A malformed metrics cache entry must not take every plugin down with it.
+
+`ResourceMetrics(**cached)` raises TypeError on a single unexpected key, and
+that exception escapes into plugin_manager, which reports it per plugin as
+"plugin <id> operation failed". Every plugin fails and the plugin system never
+finishes initialising -- the health endpoint reports
+`plugin_system: not_initialized` while the display itself keeps running.
+
+Seen on a live rig, once per plugin, continuously:
+
+    ERROR - src.plugin_system.plugin_manager - plugin geochron operation failed:
+    ResourceMetrics.__init__() got an unexpected keyword argument
+    'consecutive_failures'
+
+`consecutive_failures` belongs to plugin_health, not to metrics. How a
+health-shaped record came to sit under a plugin_metrics key on that machine is
+not established -- a restored backup that mixed two machines' caches is the
+likeliest explanation, and the same rig had one restored onto it -- but a
+loader that turns one bad cache entry into a total outage is the part worth
+fixing. plugin_health already repairs its own records field by field rather
+than trusting what is on disk.
+"""
+import logging
+from dataclasses import fields
+from unittest.mock import MagicMock
+
+import pytest
+
+from src.plugin_system.resource_monitor import PluginResourceMonitor, ResourceMetrics
+
+
+class _Cache:
+    def __init__(self, payload=None):
+        self.payload = payload
+
+    def get(self, key, max_age=None, memory_ttl=None, **kwargs):
+        return self.payload
+
+    def set(self, key, data, ttl=None, **kwargs):
+        pass
+
+
+def _monitor(payload):
+    m = PluginResourceMonitor(cache_manager=_Cache(payload))
+    m.logger = logging.getLogger("test")
+    return m
+
+
+#: What the rig actually had under the metrics key.
+HEALTH_SHAPED = {
+    "consecutive_failures": 0, "circuit_state": "closed",
+    "circuit_opened_time": None, "half_open_start_time": None,
+    "last_error": None, "last_failure_time": None,
+    "last_success_time": 1_700_000_000.0, "total_failures": 0,
+    "total_successes": 42,
+}
+
+
+def test_a_health_record_under_the_metrics_key_does_not_raise():
+    """The exact failure: it must degrade, not take the plugin system down."""
+    monitor = _monitor(HEALTH_SHAPED)
+    metrics = monitor.get_metrics(" plugin-a".strip())
+    assert isinstance(metrics, ResourceMetrics)
+
+
+def test_recognised_fields_in_a_mixed_record_are_kept():
+    """Dropping the record wholesale would lose real history unnecessarily."""
+    mixed = dict(HEALTH_SHAPED, call_count=7, memory_mb=12.5)
+    metrics = _monitor(mixed).get_metrics("plugin-b")
+    assert metrics.call_count == 7
+    assert metrics.memory_mb == 12.5
+
+
+def test_a_clean_record_still_loads_unchanged():
+    clean = {f.name: 3 for f in fields(ResourceMetrics)}
+    metrics = _monitor(clean).get_metrics("plugin-c")
+    for name in (f.name for f in fields(ResourceMetrics)):
+        assert getattr(metrics, name) == 3
+
+
+def test_unknown_fields_are_named_in_the_log(caplog):
+    """Silently discarding them would hide a real schema change."""
+    with caplog.at_level(logging.WARNING):
+        _monitor(HEALTH_SHAPED).get_metrics("plugin-d")
+    # getMessage(), not .message: the latter is only populated once a handler
+    # formats the record, so the obvious spelling silently never matches.
+    assert any("consecutive_failures" in r.getMessage() for r in caplog.records), \
+        caplog.text
+
+
+@pytest.mark.parametrize("payload", ["a string", 42, ["a", "list"]])
+def test_a_non_mapping_cache_entry_does_not_raise(payload):
+    metrics = _monitor(payload).get_metrics("plugin-e")
+    assert isinstance(metrics, ResourceMetrics)
+
+
+def test_values_of_the_wrong_type_do_not_raise():
+    """A dataclass will accept these, but a later float() on them would not."""
+    metrics = _monitor({"call_count": "not a number"}).get_metrics("plugin-f")
+    assert isinstance(metrics, ResourceMetrics)

test/test_restore_keeps_panel_hardware.py

@@ -1,104 +0,0 @@
-#!/usr/bin/env python3
-"""A restore must not repoint this device at another machine's panel.
-
-display.hardware describes the panel physically wired to this device -- cols,
-rows, chain_length, hardware_mapping, panel_type, multiplexing, the refresh
-cap. A backup carries the panel of the machine it was taken on. Restoring a
-512x64 rig's backup onto a 128x32 one used to overwrite the smaller panel's
-geometry with the larger one's, and nothing on screen explains why: the
-display just stops being right.
-
-That is not hypothetical. It happened, and the rig it happened to had to be
-reflashed.
-"""
-import json
-import sys
-from pathlib import Path
-
-sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
-
-from src.backup_manager import _restore_config_preserving_hardware  # noqa: E402
-
-BIG = {"display": {"hardware": {"cols": 128, "rows": 64, "chain_length": 4,
-                                "hardware_mapping": "adafruit-hat-pwm"},
-                   "runtime": {"gpio_slowdown": 4}},
-       "timezone": "America/New_York", "some-plugin": {"enabled": True}}
-SMALL = {"display": {"hardware": {"cols": 64, "rows": 32, "chain_length": 2,
-                                  "hardware_mapping": "regular"},
-                     "runtime": {"gpio_slowdown": 2}},
-         "timezone": "UTC"}
-
-
-def _run(tmp, keep):
-    src = tmp / "backup_config.json"; src.write_text(json.dumps(BIG))
-    dst = tmp / "config.json"; dst.write_text(json.dumps(SMALL))
-    _restore_config_preserving_hardware(src, dst, keep_hardware=keep)
-    return json.loads(dst.read_text())
-
-
-def test_local_panel_survives(tmp_path):
-    out = _run(tmp_path, keep=True)
-    hw = out["display"]["hardware"]
-    assert (hw["cols"], hw["rows"], hw["chain_length"]) == (64, 32, 2), (
-        "the restore repointed this device at the backup's panel")
-    assert hw["hardware_mapping"] == "regular", "panel wiring came from the backup"
-
-
-def test_everything_else_is_restored(tmp_path):
-    out = _run(tmp_path, keep=True)
-    assert out["timezone"] == "America/New_York", "config was not restored"
-    assert out["some-plugin"] == {"enabled": True}, "plugin config was not restored"
-    assert out["display"]["runtime"] == {"gpio_slowdown": 4}, (
-        "only display.hardware should be held back")
-
-
-def test_opting_in_takes_the_backups_panel(tmp_path):
-    out = _run(tmp_path, keep=False)
-    hw = out["display"]["hardware"]
-    assert (hw["cols"], hw["rows"], hw["chain_length"]) == (128, 64, 4)
-
-
-def test_a_device_with_no_local_hardware_takes_the_backups(tmp_path):
-    src = tmp_path / "b.json"; src.write_text(json.dumps(BIG))
-    dst = tmp_path / "c.json"; dst.write_text(json.dumps({"timezone": "UTC"}))
-    _restore_config_preserving_hardware(src, dst, keep_hardware=True)
-    out = json.loads(dst.read_text())
-    assert out["display"]["hardware"]["cols"] == 128, (
-        "nothing local to preserve, so the backup's panel should be used")
-
-
-def test_unparseable_local_config_still_restores(tmp_path):
-    src = tmp_path / "b.json"; src.write_text(json.dumps(BIG))
-    dst = tmp_path / "c.json"; dst.write_text("{ not json")
-    _restore_config_preserving_hardware(src, dst, keep_hardware=True)
-    assert json.loads(dst.read_text())["timezone"] == "America/New_York", (
-        "a restore must never fail because of this merge")
-
-
-def test_the_destination_mode_is_preserved(tmp_path):
-    """config.json is installed with a deliberate mode; a merge must not widen it.
-
-    _copy_file preserves the destination's mode and owner on purpose -- these
-    files are root-owned while the web interface running the restore is not.
-    Writing the merged result directly would have replaced that with whatever
-    the umask allowed.
-    """
-    import os
-    import stat as statmod
-    src = tmp_path / "b.json"; src.write_text(json.dumps(BIG))
-    dst = tmp_path / "c.json"; dst.write_text(json.dumps(SMALL))
-    os.chmod(dst, 0o600)
-
-    _restore_config_preserving_hardware(src, dst, keep_hardware=True)
-
-    mode = statmod.S_IMODE(os.stat(dst).st_mode)
-    assert mode == 0o600, f"restore widened config.json from 0600 to {oct(mode)}"
-    assert json.loads(dst.read_text())["display"]["hardware"]["cols"] == 64
-
-
-def test_no_scratch_file_is_left_behind(tmp_path):
-    src = tmp_path / "b.json"; src.write_text(json.dumps(BIG))
-    dst = tmp_path / "c.json"; dst.write_text(json.dumps(SMALL))
-    _restore_config_preserving_hardware(src, dst, keep_hardware=True)
-    leftovers = [p.name for p in tmp_path.iterdir() if "tmp" in p.name]
-    assert not leftovers, f"scratch files left in place: {leftovers}"