mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-04 10:18:05 +00:00
* feat(store): refuse to install a plugin that needs a newer core `ledmatrix_min_version` was decoration. The loader logged an advisory warning and continued; the store never compared the core version at all, so a routine "update" delivered a plugin that could not run. That is the gap phase B6 (the sports-unification sunset) cannot be done over: deleting a plugin's bundled fallback while nothing enforces the floor hands un-updated users a scoreboard that raises ModuleNotFoundError at load and is reported only as one line in the journal. The gate lives in install_plugin, after the manifest is on disk and before dependencies are installed. That is the earliest knowable point -- the registry carries no compatibility field, so the floor is not visible until the files are down -- and it is also the chokepoint: _reinstall_with_rollback calls install_plugin, so a refused *update* restores the version the user already had, for free. Floor resolution and the comparison move to src/plugin_system/compatibility.py, shared with the loader so the two cannot drift. Both read all four spellings published manifests use, including the deprecated `ledmatrix_min`. Refusal requires evidence. An undeclared floor, an unparseable version on either side, or a core below TRUSTWORTHY_FLOOR (2.0.0) all allow the install. That last one is deliberate and load-bearing: the v3.1.0 release reports __version__ = "1.0.0" while nearly every published manifest floors at 2.0.0, so a strict gate would lock those users out of the plugin store entirely -- much worse than the problem being solved. They stay unprotected until they update the core, which is also what fixes their version string. Verified: 782 core unit tests pass, including 25 new ones and the existing loader-warning suite unchanged (the refactor is behavior-preserving). The install tests drive the real install_plugin path with the download stubbed -- the allow and refuse cases differ only in the declared floor, so the refusal is demonstrably the gate and not an earlier bail-out. Follow-ups, deliberately not in this PR: surfacing the reason in the store UI rather than only the log, and publishing the floor in plugins.json so the store can refuse before downloading. Phase B4 in docs/SPORTS_UNIFICATION.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(store): a failed install must not destroy the plugin it replaced Found while validating the compatibility gate. `_install_plugin_impl` deletes the existing plugin directory *before* downloading, so any failure after that point leaves the user with nothing. `_reinstall_with_rollback` protects the update path exactly this way; a direct `install_plugin` had no equivalent. The gate made this reachable in a new way: a plugin whose declared floor exceeds the running core is now refused *after* the old copy is already gone. Floors are hand-written and can be over-declared, so the refusal could remove a plugin that had been working fine on that core. install_plugin is now a thin wrapper that renames any existing install aside, delegates to _install_plugin_impl, and restores it on failure -- including when the implementation raises, which is re-raised after the restore. It is a pass-through when nothing is installed and when called from _reinstall_with_rollback, which has already moved the old copy aside; a test pins that so the two mechanisms cannot start nesting. The aside name embeds '.standalone-backup-' because plugin_manager._scan_directory_for_plugins keys on exactly that substring to skip backups. A different name would have made the backup discoverable as a duplicate plugin; a test pins that too. 789 core unit tests pass, including 7 new ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * fix(store): serialize concurrent installs, and make the lock reentrant Second bug found while validating the previous commit on hardware. install_plugin's new set-aside/restore had no lock. The web UI runs Flask threaded, so a double-clicked Install button gives two threads the same plugin_id; interleaved, one thread's restore deletes the other's freshly installed copy. _reinstall_with_rollback already guards exactly this with a per-plugin lock, and install_plugin needs the same one. Taking that lock naively deadlocks. _reinstall_with_rollback holds it across its call to install_plugin, and threading.Lock is not reentrant -- so the request thread hangs forever on the standard monorepo update path (update_plugin -> _reinstall_with_rollback -> install_plugin), which is to say on every plugin update. Verified by reverting to a plain Lock: the regression test times out after 10s instead of passing. The per-plugin locks are now RLocks, and install_plugin holds one for its whole set-aside/install/restore sequence. Verified on devpi (Pi, Python 3.13.5, real registry and network): - update_plugin on an up-to-date plugin: True in 5.4s - update_plugin forced through the full reinstall-with-rollback path: True in 13.1s, correct version restored, old copy replaced, no backup directories left behind - install -> reinstall-over-existing -> failed-reinstall-restores: all pass against real downloads - 22 plugins load, no tracebacks, web API and UI 200, steady-state journal 50 lines/min 791 core unit tests pass, including 2 new concurrency tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 * ci: run the new suites, and check tag/version agreement at release time These were split out of #428/#429 because the token pushing them lacked the `workflow` scope. Folding them in here rather than opening a stacked PR -- #429 was merged into its stacked base after that base had already been squash-merged, so its content never reached main, and one such near-miss is enough. All three enrolled suites exist on this branch: test_version_consistency.py came with #428 and is on main; the other two arrive with the commits above. Enrolling them in a separate PR would have either raced with this one on test.yml or briefly pointed CI at files main did not have. - test.yml: enroll test_version_consistency, test_plugin_compatibility_gate and test_install_preserves_existing in the core unit job. Until now these 32 tests existed but nothing ran them automatically. - release-version-check.yml: run scripts/check_release_version.py on pushed v* tags and published releases, plus workflow_dispatch so a tag can be checked *before* it is created. No dependencies -- it reads src/__init__.py and CHANGELOG.md only. Verified: both workflow files parse, and the release check still passes for v3.2.0 against this tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
223 lines
9.2 KiB
Python
223 lines
9.2 KiB
Python
"""A failed (re)install must not destroy the working plugin it replaced.
|
|
|
|
`_install_plugin_impl` deletes the existing plugin directory *before* it
|
|
downloads anything, so any failure after that point used to leave the user with
|
|
nothing. The update path was protected — `_reinstall_with_rollback` renames the
|
|
old copy aside first — but a direct `install_plugin` was not, and the
|
|
compatibility gate added a new way to fail late: a plugin whose declared floor
|
|
exceeds the running core is now refused *after* the old copy is already gone.
|
|
|
|
Concretely, without the wrapper: a user on core 3.1.0 with a working
|
|
hockey-scoreboard clicks Install; the new manifest floors at 3.2.0; the gate
|
|
refuses; the plugin they had is deleted. Floors are hand-written and can be
|
|
over-declared, so this could remove a plugin that was working fine.
|
|
"""
|
|
|
|
import json
|
|
from pathlib import Path
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
from src.plugin_system.store_manager import PluginStoreManager
|
|
|
|
|
|
@pytest.fixture
|
|
def store(tmp_path):
|
|
plugins_dir = tmp_path / "plugin-repos"
|
|
plugins_dir.mkdir()
|
|
mgr = PluginStoreManager(plugins_dir=str(plugins_dir))
|
|
mgr.logger = MagicMock()
|
|
return mgr, plugins_dir
|
|
|
|
|
|
def _existing_install(plugins_dir: Path, plugin_id: str, marker: str) -> Path:
|
|
path = plugins_dir / plugin_id
|
|
path.mkdir(parents=True)
|
|
(path / "manifest.json").write_text(
|
|
json.dumps({"id": plugin_id, "name": plugin_id, "class_name": "P",
|
|
"display_modes": ["a"], "version": "1.0.0"}),
|
|
encoding="utf-8")
|
|
(path / "marker.txt").write_text(marker, encoding="utf-8")
|
|
return path
|
|
|
|
|
|
class TestFailedInstallPreservesPrevious:
|
|
def test_failed_install_restores_the_old_copy(self, store, monkeypatch):
|
|
mgr, plugins_dir = store
|
|
path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
|
|
|
monkeypatch.setattr(mgr, "_install_plugin_impl", lambda *a, **k: False)
|
|
|
|
assert mgr.install_plugin("hockey-scoreboard") is False
|
|
assert path.exists(), "the previous install must be restored"
|
|
assert (path / "marker.txt").read_text() == "the-original"
|
|
|
|
def test_raising_install_restores_and_reraises(self, store, monkeypatch):
|
|
mgr, plugins_dir = store
|
|
path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
|
|
|
def boom(*a, **k):
|
|
raise RuntimeError("network died mid-install")
|
|
|
|
monkeypatch.setattr(mgr, "_install_plugin_impl", boom)
|
|
|
|
with pytest.raises(RuntimeError):
|
|
mgr.install_plugin("hockey-scoreboard")
|
|
assert path.exists()
|
|
assert (path / "marker.txt").read_text() == "the-original"
|
|
|
|
def test_successful_install_clears_the_backup(self, store, monkeypatch):
|
|
mgr, plugins_dir = store
|
|
_existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
|
|
|
def succeed(plugin_id, branch=None):
|
|
_existing_install(plugins_dir, plugin_id, "the-new-one")
|
|
return True
|
|
|
|
monkeypatch.setattr(mgr, "_install_plugin_impl", succeed)
|
|
|
|
assert mgr.install_plugin("hockey-scoreboard") is True
|
|
assert (plugins_dir / "hockey-scoreboard" / "marker.txt").read_text() == "the-new-one"
|
|
leftovers = [p.name for p in plugins_dir.iterdir() if "backup" in p.name]
|
|
assert not leftovers, f"backup left behind: {leftovers}"
|
|
|
|
def test_backup_name_is_invisible_to_plugin_discovery(self, store, monkeypatch):
|
|
"""A backup that discovery can see becomes a duplicate plugin entry;
|
|
the marker '.standalone-backup-' is what makes it skip."""
|
|
mgr, plugins_dir = store
|
|
_existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
|
|
|
seen = {}
|
|
|
|
def capture(plugin_id, branch=None):
|
|
seen["dirs"] = sorted(p.name for p in plugins_dir.iterdir())
|
|
return False
|
|
|
|
monkeypatch.setattr(mgr, "_install_plugin_impl", capture)
|
|
mgr.install_plugin("hockey-scoreboard")
|
|
|
|
backups = [d for d in seen["dirs"] if d != "hockey-scoreboard"]
|
|
assert backups, "expected the old copy to be set aside during install"
|
|
for name in backups:
|
|
assert ".standalone-backup-" in name, (
|
|
f"{name} would be picked up by "
|
|
"plugin_manager._scan_directory_for_plugins as a real plugin")
|
|
|
|
def test_fresh_install_is_a_pass_through(self, store, monkeypatch):
|
|
"""Nothing installed means nothing to protect; don't create stray dirs."""
|
|
mgr, plugins_dir = store
|
|
calls = []
|
|
monkeypatch.setattr(
|
|
mgr, "_install_plugin_impl",
|
|
lambda *a, **k: calls.append(a) or True)
|
|
|
|
assert mgr.install_plugin("brand-new") is True
|
|
assert calls, "the implementation must still be called"
|
|
assert list(plugins_dir.iterdir()) == []
|
|
|
|
def test_stale_backup_from_a_crash_does_not_block(self, store, monkeypatch):
|
|
mgr, plugins_dir = store
|
|
_existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
|
stale = plugins_dir / "hockey-scoreboard.standalone-backup-preinstall"
|
|
stale.mkdir()
|
|
(stale / "junk.txt").write_text("from a previous crash", encoding="utf-8")
|
|
|
|
monkeypatch.setattr(mgr, "_install_plugin_impl", lambda *a, **k: False)
|
|
|
|
assert mgr.install_plugin("hockey-scoreboard") is False
|
|
assert (plugins_dir / "hockey-scoreboard" / "marker.txt").read_text() == "the-original"
|
|
|
|
|
|
class TestUpdatePathStillWorks:
|
|
def test_reinstall_with_rollback_is_not_double_wrapped(self, store, monkeypatch):
|
|
"""_reinstall_with_rollback moves the plugin aside itself, so by the
|
|
time install_plugin runs there is nothing at the original path and the
|
|
wrapper must be a pass-through rather than staging a second backup."""
|
|
mgr, plugins_dir = store
|
|
path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
|
|
|
observed = {}
|
|
|
|
def impl(plugin_id, branch=None):
|
|
observed["dirs"] = sorted(p.name for p in plugins_dir.iterdir())
|
|
return False
|
|
|
|
monkeypatch.setattr(mgr, "_install_plugin_impl", impl)
|
|
|
|
assert mgr._reinstall_with_rollback("hockey-scoreboard", path) is False
|
|
# Exactly one aside directory existed during the attempt — rollback's.
|
|
assert observed["dirs"] == ["hockey-scoreboard.standalone-backup-migrating"]
|
|
# And the user still has their plugin.
|
|
assert (plugins_dir / "hockey-scoreboard" / "marker.txt").read_text() == "the-original"
|
|
|
|
|
|
class TestConcurrency:
|
|
"""The web UI runs Flask threaded, so a double-clicked Install button puts
|
|
two threads on the same plugin_id. `_reinstall_with_rollback` already
|
|
guarded against this; the install wrapper has to as well, or one thread's
|
|
restore deletes the other's freshly installed copy."""
|
|
|
|
def test_rollback_calling_install_does_not_deadlock(self, store, monkeypatch):
|
|
"""The rollback path holds the per-plugin lock across its call to
|
|
install_plugin. A non-reentrant lock would hang the request thread
|
|
forever — this test would time out rather than fail."""
|
|
import threading
|
|
|
|
mgr, plugins_dir = store
|
|
path = _existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
|
monkeypatch.setattr(
|
|
mgr, "_install_plugin_impl",
|
|
lambda pid, branch=None: bool(_existing_install(plugins_dir, pid, "new")))
|
|
|
|
done = threading.Event()
|
|
result = {}
|
|
|
|
def run():
|
|
result["ok"] = mgr._reinstall_with_rollback("hockey-scoreboard", path)
|
|
done.set()
|
|
|
|
t = threading.Thread(target=run, daemon=True)
|
|
t.start()
|
|
assert done.wait(timeout=10), (
|
|
"install_plugin deadlocked when called from _reinstall_with_rollback "
|
|
"— the per-plugin lock must be reentrant"
|
|
)
|
|
assert result["ok"] is True
|
|
|
|
def test_concurrent_installs_serialize(self, store, monkeypatch):
|
|
"""Two threads installing the same plugin must not interleave their
|
|
set-aside/restore, and the survivor must be a complete install."""
|
|
import threading
|
|
|
|
mgr, plugins_dir = store
|
|
_existing_install(plugins_dir, "hockey-scoreboard", "the-original")
|
|
|
|
in_flight = []
|
|
overlap = []
|
|
|
|
def slow_impl(plugin_id, branch=None):
|
|
in_flight.append(1)
|
|
if len(in_flight) > 1:
|
|
overlap.append(1)
|
|
threading.Event().wait(0.05)
|
|
_existing_install(plugins_dir, plugin_id, "installed")
|
|
in_flight.pop()
|
|
return True
|
|
|
|
monkeypatch.setattr(mgr, "_install_plugin_impl", slow_impl)
|
|
|
|
threads = [threading.Thread(target=mgr.install_plugin,
|
|
args=("hockey-scoreboard",), daemon=True)
|
|
for _ in range(2)]
|
|
for t in threads:
|
|
t.start()
|
|
for t in threads:
|
|
t.join(timeout=10)
|
|
assert not t.is_alive(), "concurrent install hung"
|
|
|
|
assert not overlap, "two installs of the same plugin ran concurrently"
|
|
assert (plugins_dir / "hockey-scoreboard" / "marker.txt").exists()
|
|
leftovers = [p.name for p in plugins_dir.iterdir() if "backup" in p.name]
|
|
assert not leftovers, f"backup left behind: {leftovers}"
|