fix(plugin-system): unload/update race, failed-load cleanup, limits validation, schema lookup, install rollback (#653)

* fix(plugin-system): unload/update race, failed-load module cleanup, limits validation, schema lookup, install rollback, op-queue dedupe - unload_plugin takes the per-plugin lock (5s bounded) before cleanup(), and an update() that finishes after its plugin was unloaded no longer sets the state back to ENABLED. - A load that fails after import drops plugin_<id> and its submodules and forgets its manager fonts, so a fixed plugin reloads new code. - Resource limits are validated as non-negative numbers: 400 at POST /plugins/limits, bad cached records ignored with one warning. Route docstrings note health/metrics reset and limits only change the web process's view. - SchemaManager.get_schema_path resolves each search dir via resolve_plugin_dir (manifest id, ledmatrix-<id>) before the literal paths; plugins/ still before plugin-repos/. Misses cached 30s and logged once at DEBUG. - install_from_url sets an existing copy aside and restores it if the move fails, under the per-plugin reinstall lock. - Operation queue refuses a second pending op for a plugin and trims _operations with history. - get_vegas_render_width reads display_manager.width first. - get_logger in store/schema/health/resource/saved_repositories; UTF-8 reads in store_manager and state_manager. - Docs: update_interval precedence (manifest over config) stated where users are told to set it in config. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(web): build the limits 400 message from the field name, not an exception CodeQL flagged str(e) flowing into the response. invalid_limit_field() returns the offending field without raising, and limits_from_dict uses it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 14:25:08 +00:00 · 2026-09-28 10:41:40 -04:00
parent 0e9e2cabba
commit c00bf5e8e6
24 changed files with 869 additions and 49 deletions
@@ -254,6 +254,51 @@ class TestFailurePaths:
        assert target_id not in pm._pending_updates
        assert pm.state_manager.get_state(target_id) == PluginState.UNLOADED

+    def test_unload_waits_for_in_flight_update(self, pm):
+        """unload_plugin() must not run cleanup() while update() is still
+        executing on the same instance -- it waits on the plugin lock."""
+        observed = {}
+
+        class CleanupPlugin(SlowPlugin):
+            def cleanup(self):
+                observed['in_update_at_cleanup'] = self.in_update
+
+        plugin = CleanupPlugin(update_seconds=0.5)
+        plugin_id = _install(pm, plugin)
+        pm._enqueue_update(plugin_id, time.time())
+        deadline = time.monotonic() + 2
+        while not plugin.in_update and time.monotonic() < deadline:
+            time.sleep(0.01)
+        assert plugin.in_update
+
+        assert pm.unload_plugin(plugin_id) is True
+        assert observed == {'in_update_at_cleanup': False}
+
+    def test_update_finishing_after_unload_does_not_resurrect(self, pm):
+        """When unload gives up waiting for a hung update(), that update's
+        eventual completion must not flip the cleared state back to ENABLED."""
+        pm.UNLOAD_LOCK_TIMEOUT = 0.05
+        plugin = SlowPlugin(update_seconds=0.5)
+        plugin_id = _install(pm, plugin)
+        pm._enqueue_update(plugin_id, time.time())
+        deadline = time.monotonic() + 2
+        while not plugin.in_update and time.monotonic() < deadline:
+            time.sleep(0.01)
+        assert plugin.in_update
+
+        assert pm.unload_plugin(plugin_id) is True
+        deadline = time.monotonic() + 3
+        while plugin.update_calls and plugin.in_update and time.monotonic() < deadline:
+            time.sleep(0.02)
+        time.sleep(0.2)  # let _finish() run
+
+        assert pm.state_manager.get_state(plugin_id) == PluginState.UNLOADED
+        assert plugin_id not in pm.plugin_last_update
+        assert plugin_id not in pm._pending_updates
+        lock = pm.get_plugin_lock(plugin_id)
+        assert lock.acquire(blocking=False) is True
+        lock.release()
+

 class TestKillSwitch:
    def test_synchronous_mode_blocks_like_before(self, pm):
@@ -0,0 +1,48 @@
+"""BasePlugin.get_vegas_render_width reads display_manager.width first.
+
+CLAUDE.md asks plugins to size themselves from ``display_manager.width`` --
+not ``display_manager.matrix.width`` -- because ``matrix`` is None when
+hardware init fails and the property falls back to the canvas size. The base
+class's own helper read ``matrix.width`` first. For the real DisplayManager
+the two agree whenever there is a matrix, so this pins the documented order
+with a display manager where they differ.
+"""
+
+from types import SimpleNamespace
+
+from src.plugin_system.base_plugin import BasePlugin
+
+
+class _Plugin(BasePlugin):
+    def update(self):
+        pass
+
+    def display(self, force_clear=False):
+        pass
+
+
+def _plugin(display_manager):
+    plugin = object.__new__(_Plugin)
+    plugin.display_manager = display_manager
+    return plugin
+
+
+def test_prefers_display_manager_width_over_matrix_width():
+    dm = SimpleNamespace(width=64, matrix=SimpleNamespace(width=128))
+    assert _plugin(dm).get_vegas_render_width() == 64
+
+
+def test_uses_display_manager_width_when_matrix_is_none():
+    dm = SimpleNamespace(width=96, matrix=None)
+    assert _plugin(dm).get_vegas_render_width() == 96
+
+
+def test_falls_back_to_matrix_width_without_a_width():
+    dm = SimpleNamespace(matrix=SimpleNamespace(width=80))
+    assert _plugin(dm).get_vegas_render_width() == 80
+
+
+def test_vegas_request_still_wins():
+    plugin = _plugin(SimpleNamespace(width=192, matrix=None))
+    plugin._vegas_render_width = 100
+    assert plugin.get_vegas_render_width() == 100
@@ -29,6 +29,11 @@ class TestParseSemver:
    def test_leading_v_tolerated(self):
        assert parse_semver("v3.2.1") == (3, 2, 1)

+    def test_non_decimal_digit_is_unparseable(self):
+        # str.isdigit() accepts "²" but int() does not: the ValueError
+        # branch is reachable and must keep returning None, not raise.
+        assert parse_semver("1.².0") is None
+
    def test_prerelease_suffix_stripped(self):
        # "3.2.0-rc1" must NOT parse as (3, 2, 1) — a release candidate must
        # not rank above its own release.
@@ -0,0 +1,71 @@
+"""PluginOperationQueue refuses a second queued op per plugin and stays bounded.
+
+enqueue_operation only checked _active_operations, which holds the operation
+that is *running*. While a plugin's first operation still waited in the queue
+(the worker busy with another plugin), a second one for the same plugin was
+accepted and both ran back to back. And _operations kept every operation ever
+enqueued, although the history beside it was trimmed to max_history.
+"""
+
+import threading
+import time
+
+import pytest
+
+from src.plugin_system.operation_queue import PluginOperationQueue
+from src.plugin_system.operation_types import OperationStatus, OperationType
+
+
+@pytest.fixture
+def op_queue():
+    q = PluginOperationQueue(max_history=3)
+    yield q
+    q.shutdown()
+
+
+def _wait_for(predicate, timeout=5.0):
+    deadline = time.monotonic() + timeout
+    while not predicate() and time.monotonic() < deadline:
+        time.sleep(0.01)
+    return predicate()
+
+
+def test_second_pending_operation_for_a_plugin_is_refused(op_queue):
+    release = threading.Event()
+    started = threading.Event()
+
+    def blocker(op):
+        started.set()
+        release.wait(5)
+        return {"success": True}
+
+    op_queue.enqueue_operation(OperationType.INSTALL, "busy", operation_callback=blocker)
+    assert started.wait(5)
+
+    first = op_queue.enqueue_operation(
+        OperationType.INSTALL, "demo", operation_callback=lambda op: {"success": True})
+    assert op_queue.get_operation_status(first).status == OperationStatus.PENDING
+
+    with pytest.raises(ValueError, match="already has an active operation"):
+        op_queue.enqueue_operation(
+            OperationType.INSTALL, "demo", operation_callback=lambda op: {"success": True})
+
+    release.set()
+    assert _wait_for(lambda: op_queue.get_operation_status(first).status
+                     == OperationStatus.COMPLETED)
+    # Once it has finished, the plugin accepts a new operation again.
+    op_queue.enqueue_operation(OperationType.UPDATE, "demo")
+
+
+def test_operations_map_is_trimmed_with_history(op_queue):
+    ids = [op_queue.enqueue_operation(OperationType.INSTALL, f"p{i}",
+                                      operation_callback=lambda op: {"success": True})
+           for i in range(8)]
+    assert _wait_for(lambda: all(
+        (op_queue.get_operation_status(i) is None
+         or op_queue.get_operation_status(i).status == OperationStatus.COMPLETED)
+        for i in ids) and len(op_queue.get_operation_history()) == 3)
+
+    assert len(op_queue._operations) == 3
+    kept = {op.operation_id for op in op_queue.get_operation_history()}
+    assert set(op_queue._operations) == kept
@@ -0,0 +1,75 @@
+"""A load that fails after the plugin module was imported must not leave
+that module behind.
+
+PluginLoader.load_module() reuses ``plugin_<id>`` from sys.modules. When
+instantiation or validate_config() failed, the half-loaded module stayed
+there (and in the loader's ``_loaded_modules``), so after the user fixed the
+plugin, the next load kept running the old, broken code until a restart.
+Font registrations the failed instance made stayed listed too.
+"""
+
+import json
+import sys
+from unittest.mock import MagicMock
+
+import pytest
+
+from src.plugin_system.plugin_manager import PluginManager
+from src.plugin_system.plugin_state import PluginState
+
+PLUGIN_ID = "failed-load-demo"
+MODULE_NAME = "plugin_failed_load_demo"
+
+_BROKEN_INIT = '''
+class Demo:
+    def __init__(self, plugin_id, config, display_manager, cache_manager, plugin_manager):
+        raise RuntimeError("broken constructor")
+'''
+
+_BAD_CONFIG = '''
+class Demo:
+    VERSION = "bad-config"
+    def __init__(self, plugin_id, config, display_manager, cache_manager, plugin_manager):
+        pass
+    def validate_config(self):
+        return False
+'''
+
+_FIXED = '''
+class Demo:
+    VERSION = "fixed"
+    def __init__(self, plugin_id, config, display_manager, cache_manager, plugin_manager):
+        self.enabled = True
+'''
+
+
+@pytest.fixture
+def plugin_env(tmp_path):
+    plugins_dir = tmp_path / "plugins"
+    plugin_dir = plugins_dir / PLUGIN_ID
+    plugin_dir.mkdir(parents=True)
+    manifest = {"id": PLUGIN_ID, "name": "Demo", "class_name": "Demo",
+                "entry_point": "manager.py"}
+    (plugin_dir / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8")
+    manager = PluginManager(plugins_dir=str(plugins_dir))
+    manager.font_manager = MagicMock()
+    manager.plugin_manifests[PLUGIN_ID] = manifest
+    yield manager, plugin_dir
+    sys.modules.pop(MODULE_NAME, None)
+
+
+@pytest.mark.parametrize("first_source", [_BROKEN_INIT, _BAD_CONFIG],
+                         ids=["instantiate-fails", "validate-config-fails"])
+def test_fixed_plugin_loads_new_code_after_failed_load(plugin_env, first_source):
+    pm, plugin_dir = plugin_env
+    (plugin_dir / "manager.py").write_text(first_source, encoding="utf-8")
+
+    assert pm.load_plugin(PLUGIN_ID) is False
+    assert pm.state_manager.get_state(PLUGIN_ID) == PluginState.ERROR
+    assert MODULE_NAME not in sys.modules
+    assert PLUGIN_ID not in pm.plugin_loader._loaded_modules
+    pm.font_manager.forget_manager_fonts.assert_called_with(PLUGIN_ID)
+
+    (plugin_dir / "manager.py").write_text(_FIXED, encoding="utf-8")
+    assert pm.load_plugin(PLUGIN_ID) is True
+    assert pm.plugins[PLUGIN_ID].VERSION == "fixed"
@@ -0,0 +1,62 @@
+"""Plugin-system JSON files are read as UTF-8, whatever the locale says.
+
+store_manager (install_from_url's manifest, the secrets file) and
+state_manager (plugin_state.json) opened text files without an encoding, so
+the platform default applied. A manifest written in UTF-8 with a non-ASCII
+name then read as mojibake -- or raised UnicodeDecodeError -- on a host
+whose locale encoding is not UTF-8 (Windows' cp1252; a Pi with LANG=C).
+On a UTF-8 host these pass either way; they fail on old code where the
+default encoding differs.
+"""
+
+import json
+
+import pytest
+
+from src.plugin_system.state_manager import PluginStateManager
+from src.plugin_system.store_manager import PluginStoreManager
+
+NAME = "Météo Á"  # "Á" is C3 81 in UTF-8; 0x81 is undefined in cp1252
+
+
+@pytest.fixture
+def store(tmp_path, monkeypatch):
+    mgr = PluginStoreManager(
+        plugins_dir=str(tmp_path / "plugins"),
+        uninstalled_registry_path=str(tmp_path / "uninstalled.json"))
+    mgr.plugins_dir.mkdir(parents=True, exist_ok=True)
+    monkeypatch.setattr(mgr, "_install_dependencies", lambda *a, **k: True)
+
+    def fake_clone(repo_url, target, branches):
+        target = type(mgr.plugins_dir)(target)
+        target.mkdir(parents=True, exist_ok=True)
+        manifest = {"id": "meteo", "name": NAME, "class_name": "P",
+                    "display_modes": ["meteo"], "version": "1.0.0"}
+        (target / "manifest.json").write_bytes(
+            json.dumps(manifest, ensure_ascii=False).encode("utf-8"))
+        (target / "manager.py").write_text("class P: pass\n")
+        return "main"
+
+    monkeypatch.setattr(mgr, "_install_via_git", fake_clone)
+    return mgr
+
+
+def test_install_from_url_reads_a_utf8_manifest(store):
+    result = store.install_from_url("https://github.com/x/y")
+    assert result["success"] is True
+    assert result["name"] == NAME
+    written = json.loads(
+        (store.plugins_dir / "meteo" / "manifest.json").read_bytes().decode("utf-8"))
+    assert written["name"] == NAME
+
+
+def test_state_manager_loads_a_utf8_state_file(tmp_path):
+    state_file = tmp_path / "plugin_state.json"
+    state_file.write_bytes(json.dumps({
+        "version": 1,
+        "states": {"meteo": {"plugin_id": "meteo", "status": "installed",
+                             "enabled": True, "metadata": {"label": NAME}}},
+    }, ensure_ascii=False).encode("utf-8"))
+
+    mgr = PluginStateManager(state_file=str(state_file))
+    assert mgr.get_plugin_state("meteo").metadata["label"] == NAME
@@ -0,0 +1,81 @@
+"""Resource limits are validated before the monitor uses them.
+
+POST /plugins/limits/<id> built ResourceLimits straight from the request
+JSON, and get_limits() did ``ResourceLimits(**cached)``. A dataclass does not
+check its annotations, so ``{"max_execution_time": "5"}`` was stored as a
+string and every later monitored update() of that plugin raised TypeError
+comparing a float with it -- and an unknown key in the cached record raised
+TypeError on load.
+"""
+
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from src.plugin_system.resource_monitor import (  # noqa: E402
+    PluginResourceMonitor,
+)
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+
+class SharedCache:
+    def __init__(self):
+        self.entries = {}
+
+    def get(self, key, max_age=None, memory_ttl=None):
+        return self.entries.get(key)
+
+    def set(self, key, value, *args, **kwargs):
+        self.entries[key] = value
+
+    def delete(self, key):
+        self.entries.pop(key, None)
+
+
+@pytest.fixture
+def shared_cache(api_v3_module):
+    cache = SharedCache()
+    api_v3_module.api_v3.plugin_manager.resource_monitor = PluginResourceMonitor(
+        cache, enable_monitoring=False)
+    return cache
+
+
+@pytest.mark.parametrize("body", [
+    {"max_execution_time": "5"},
+    {"max_memory_mb": -1},
+    {"max_cpu_percent": True},
+    {"warning_threshold": "high"},
+    {"max_execution_time": [1]},
+])
+def test_post_rejects_bad_limits_with_400(api_v3_client, shared_cache, body):
+    resp = api_v3_client.post("/api/v3/plugins/limits/weather", json=body)
+    assert resp.status_code == 400
+    assert resp.get_json()["status"] == "error"
+    assert "plugin_limits:weather" not in shared_cache.entries
+
+
+def test_post_accepts_numbers_and_nulls(api_v3_client, shared_cache):
+    resp = api_v3_client.post("/api/v3/plugins/limits/weather", json={
+        "max_memory_mb": 50, "max_cpu_percent": None, "max_execution_time": 5.0})
+    assert resp.status_code == 200
+    data = api_v3_client.get("/api/v3/plugins/limits/weather").get_json()["data"]
+    assert data == {"max_memory_mb": 50, "max_cpu_percent": None,
+                    "max_execution_time": 5.0, "warning_threshold": 0.8}
+
+
+@pytest.mark.parametrize("cached", [
+    {"max_execution_time": "5"},
+    {"max_execution_time": 5.0, "consecutive_failures": 3},
+])
+def test_bad_cached_limits_do_not_break_monitored_calls(cached):
+    cache = MagicMock()
+    cache.get.side_effect = lambda key, **kw: cached if key == "plugin_limits:p" else None
+    mon = PluginResourceMonitor(cache, enable_monitoring=False)
+
+    # Unknown keys are dropped; a malformed value means "no limits".
+    assert mon.monitor_call("p", lambda: "ok") == "ok"
+    assert mon.monitor_call("p", lambda: "ok") == "ok"
@@ -0,0 +1,68 @@
+"""SchemaManager.get_schema_path resolves plugin directories like the loader.
+
+It only tried ``<dir>/<plugin_id>``, while the loader (plugin_dirs.py) also
+finds a plugin by its manifest ``id`` and under ``ledmatrix-<id>``. A plugin
+installed under either of those loaded fine but had no settings form and was
+never schema-validated. And every lookup of a plugin with no schema logged a
+WARNING, once per call.
+"""
+
+import json
+import logging
+
+from src.plugin_system.schema_manager import SchemaManager
+
+SCHEMA = {"type": "object", "properties": {"enabled": {"type": "boolean"}}}
+
+
+def _write_plugin(base, dir_name, plugin_id, schema=True):
+    plugin_dir = base / dir_name
+    plugin_dir.mkdir(parents=True)
+    (plugin_dir / "manifest.json").write_text(json.dumps({"id": plugin_id}))
+    if schema:
+        (plugin_dir / "config_schema.json").write_text(json.dumps(SCHEMA))
+    return plugin_dir
+
+
+def test_finds_schema_in_ledmatrix_prefixed_dir(tmp_path):
+    configured = tmp_path / "plugin-repos"
+    plugin_dir = _write_plugin(configured, "ledmatrix-stocks", "stocks")
+    sm = SchemaManager(plugins_dir=configured, project_root=tmp_path)
+    assert sm.get_schema_path("stocks") == plugin_dir / "config_schema.json"
+
+
+def test_finds_schema_by_manifest_id(tmp_path):
+    configured = tmp_path / "plugin-repos"
+    plugin_dir = _write_plugin(configured, "some-other-name", "weather")
+    sm = SchemaManager(plugins_dir=configured, project_root=tmp_path)
+    assert sm.get_schema_path("weather") == plugin_dir / "config_schema.json"
+
+
+def test_plugins_dir_still_wins_over_plugin_repos(tmp_path):
+    in_plugins = _write_plugin(tmp_path / "plugins", "ledmatrix-dupe", "dupe")
+    _write_plugin(tmp_path / "plugin-repos", "dupe", "dupe")
+    sm = SchemaManager(plugins_dir=None, project_root=tmp_path)
+    assert sm.get_schema_path("dupe") == in_plugins / "config_schema.json"
+
+
+def test_miss_is_logged_once_at_debug_and_cached(tmp_path, caplog):
+    _write_plugin(tmp_path / "plugin-repos", "noschema", "noschema", schema=False)
+    sm = SchemaManager(plugins_dir=tmp_path / "plugin-repos", project_root=tmp_path,
+                       logger=logging.getLogger("test_schema_path_resolution"))
+    with caplog.at_level(logging.DEBUG, logger="test_schema_path_resolution"):
+        for _ in range(3):
+            assert sm.get_schema_path("noschema") is None
+    misses = [r for r in caplog.records if "Schema file not found" in r.getMessage()]
+    assert len(misses) == 1
+    assert misses[0].levelno == logging.DEBUG
+
+
+def test_invalidate_cache_forgets_a_miss(tmp_path):
+    configured = tmp_path / "plugin-repos"
+    plugin_dir = _write_plugin(configured, "later", "later", schema=False)
+    sm = SchemaManager(plugins_dir=configured, project_root=tmp_path)
+    assert sm.get_schema_path("later") is None
+
+    (plugin_dir / "config_schema.json").write_text(json.dumps(SCHEMA))
+    sm.invalidate_cache("later")
+    assert sm.get_schema_path("later") == plugin_dir / "config_schema.json"
@@ -0,0 +1,86 @@
+"""install_from_url keeps the installed copy until the new one is in place.
+
+It deleted the existing plugin directory and then moved the download over it,
+with no way back: a move that failed part-way left the user with no plugin at
+all. And it did so outside the per-plugin reinstall lock install_plugin()
+takes, so two overlapping installs of one id could interleave.
+"""
+
+import json
+import threading
+
+import pytest
+
+from src.plugin_system import store_manager as store_module
+from src.plugin_system.plugin_dirs import BACKUP_MARKER
+from src.plugin_system.store_manager import PluginStoreManager
+
+MANIFEST = {
+    "id": "demo", "name": "Demo", "class_name": "P",
+    "display_modes": ["demo"], "version": "2.0.0",
+}
+
+
+@pytest.fixture
+def store(tmp_path, monkeypatch):
+    mgr = PluginStoreManager(
+        plugins_dir=str(tmp_path / "plugins"),
+        uninstalled_registry_path=str(tmp_path / "uninstalled.json"))
+    mgr.plugins_dir.mkdir(parents=True, exist_ok=True)
+    monkeypatch.setattr(mgr, "_install_dependencies", lambda *a, **k: True)
+
+    def fake_clone(repo_url, target, branches):
+        target = type(mgr.plugins_dir)(target)
+        target.mkdir(parents=True, exist_ok=True)
+        (target / "manifest.json").write_text(json.dumps(MANIFEST))
+        (target / "manager.py").write_text("NEW = True\n")
+        return "main"
+
+    monkeypatch.setattr(mgr, "_install_via_git", fake_clone)
+    old = mgr.plugins_dir / "demo"
+    old.mkdir()
+    (old / "manager.py").write_text("OLD = True\n")
+    return mgr
+
+
+def _leftover_backups(store):
+    return [p for p in store.plugins_dir.iterdir() if BACKUP_MARKER in p.name]
+
+
+def test_failed_move_restores_the_existing_install(store, monkeypatch):
+    def broken_move(src, dst):
+        raise OSError("disk full")
+
+    monkeypatch.setattr(store_module.shutil, "move", broken_move)
+
+    result = store.install_from_url("https://github.com/x/y")
+
+    assert result["success"] is False
+    assert (store.plugins_dir / "demo" / "manager.py").read_text() == "OLD = True\n"
+    assert _leftover_backups(store) == []
+
+
+def test_successful_replace_leaves_no_backup(store):
+    result = store.install_from_url("https://github.com/x/y")
+
+    assert result["success"] is True
+    assert (store.plugins_dir / "demo" / "manager.py").read_text() == "NEW = True\n"
+    assert _leftover_backups(store) == []
+
+
+def test_replace_happens_under_the_reinstall_lock(store, monkeypatch):
+    real_move = store_module.shutil.move
+    seen = {}
+
+    def spying_move(src, dst):
+        lock = store._get_reinstall_lock("demo")
+        probe = threading.Thread(
+            target=lambda: seen.setdefault("free", lock.acquire(blocking=False)))
+        probe.start()
+        probe.join()
+        return real_move(src, dst)
+
+    monkeypatch.setattr(store_module.shutil, "move", spying_move)
+
+    assert store.install_from_url("https://github.com/x/y")["success"] is True
+    assert seen == {"free": False}