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>
This commit is contained in:
Chuck
@@ -0,0 +1,102 @@
|
||||
"""One place that answers "can this plugin run on this core?".
|
||||
|
||||
Two callers ask that question and they must not drift apart:
|
||||
|
||||
- `PluginLoader._warn_if_incompatible` — at load time, **advisory**. A plugin
|
||||
already on disk keeps loading regardless, because the guarded-import pattern
|
||||
means most incompatibilities degrade rather than break.
|
||||
- `PluginStoreManager.install_plugin` — at install/update time, **blocking**.
|
||||
This is the point where refusing costs the user nothing (they keep the
|
||||
version they already had) and allowing can cost them a plugin that fails to
|
||||
load with only a log line to explain it.
|
||||
|
||||
## The trustworthiness problem
|
||||
|
||||
The core's own `__version__` has not always been right. `v3.1.0` was tagged
|
||||
2026-05-31 while `src/__init__.py` still said `"1.0.0"`; the bump landed
|
||||
2026-07-12. Devices installed from that release report `1.0.0` — below the
|
||||
floor that essentially every published plugin declares.
|
||||
|
||||
So a core reporting a version below `TRUSTWORTHY_FLOOR` is treated as
|
||||
**unknown, not old**: it neither warns nor blocks. Blocking on it would be far
|
||||
worse than the problem being solved — nearly every manifest in the ecosystem
|
||||
floors at `2.0.0`, so a strict gate would stop those users installing *any*
|
||||
plugin. They are unprotected until they update the core, which is also what
|
||||
fixes their version string. See `docs/SPORTS_UNIFICATION.md`, phase B4.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import Any, Dict, Optional, Tuple
|
||||
|
||||
# Below this, the core's self-reported version is not evidence of anything.
|
||||
# See the module docstring.
|
||||
TRUSTWORTHY_FLOOR: Tuple[int, int, int] = (2, 0, 0)
|
||||
|
||||
|
||||
def parse_semver(value: Any) -> Optional[Tuple[int, int, int]]:
|
||||
"""Parse ``X.Y.Z`` (extra parts and suffixes ignored) into a comparable
|
||||
3-tuple, or ``None`` when unparseable. A leading ``v`` is tolerated."""
|
||||
if not isinstance(value, str):
|
||||
return None
|
||||
parts = value.strip().lstrip('v').split('.')
|
||||
try:
|
||||
nums = [int(''.join(ch for ch in p if ch.isdigit()) or 0) for p in parts[:3]]
|
||||
except ValueError:
|
||||
return None
|
||||
while len(nums) < 3:
|
||||
nums.append(0)
|
||||
return tuple(nums) # type: ignore[return-value]
|
||||
|
||||
|
||||
def declared_min_version(manifest: Dict[str, Any]) -> Optional[str]:
|
||||
"""The core version this plugin says it needs, or ``None`` if it doesn't say.
|
||||
|
||||
Checked in order of specificity. `ledmatrix_min` is the deprecated spelling
|
||||
of `ledmatrix_min_version` (`store_manager._validate_manifest_fields` flags
|
||||
it); both are read because a large share of published manifests still carry
|
||||
the old one.
|
||||
"""
|
||||
declared = (
|
||||
manifest.get('min_ledmatrix_version')
|
||||
or (manifest.get('requires') or {}).get('min_ledmatrix_version')
|
||||
)
|
||||
if declared:
|
||||
return declared
|
||||
|
||||
versions = manifest.get('versions') or []
|
||||
if versions and isinstance(versions[0], dict):
|
||||
return (versions[0].get('ledmatrix_min_version')
|
||||
or versions[0].get('ledmatrix_min'))
|
||||
return None
|
||||
|
||||
|
||||
def check(manifest: Dict[str, Any], core_version: str) -> Tuple[bool, Optional[str]]:
|
||||
"""Return ``(compatible, reason)``.
|
||||
|
||||
``compatible`` is False **only** when the plugin declares a parseable floor,
|
||||
the core reports a parseable and trustworthy version, and the floor is
|
||||
genuinely above it. Every uncertain case resolves to compatible: an
|
||||
undeclared floor, an unparseable version on either side, or a core whose
|
||||
version is below `TRUSTWORTHY_FLOOR`. Refusing on a guess would break
|
||||
working installs, which is the more expensive mistake here.
|
||||
|
||||
``reason`` is user-facing text, present only when incompatible.
|
||||
"""
|
||||
declared = declared_min_version(manifest)
|
||||
needed = parse_semver(declared)
|
||||
if needed is None:
|
||||
return True, None
|
||||
|
||||
current = parse_semver(core_version)
|
||||
if current is None or current < TRUSTWORTHY_FLOOR:
|
||||
return True, None
|
||||
|
||||
if needed > current:
|
||||
name = manifest.get('name') or manifest.get('id') or 'This plugin'
|
||||
return False, (
|
||||
f"{name} requires LEDMatrix {declared} or newer, but this system is "
|
||||
f"running {core_version}. Update LEDMatrix first, then install it."
|
||||
)
|
||||
|
||||
return True, None
|
||||
@@ -702,34 +702,25 @@ class PluginLoader:
|
||||
newer than the running core. Advisory only — never raises — so a
|
||||
plugin that guards optional features with try/except keeps working.
|
||||
"""
|
||||
declared = (
|
||||
manifest.get('min_ledmatrix_version')
|
||||
or manifest.get('requires', {}).get('min_ledmatrix_version')
|
||||
)
|
||||
if not declared:
|
||||
versions = manifest.get('versions') or []
|
||||
if versions and isinstance(versions[0], dict):
|
||||
declared = (versions[0].get('ledmatrix_min_version')
|
||||
or versions[0].get('ledmatrix_min'))
|
||||
needed = self._parse_semver(declared)
|
||||
if needed is None:
|
||||
from src import __version__ as core_version
|
||||
from src.plugin_system import compatibility
|
||||
|
||||
compatible, _reason = compatibility.check(manifest, core_version)
|
||||
if compatible:
|
||||
# Distinguish "fine" from "couldn't tell" for anyone reading logs:
|
||||
# a core below the trustworthy floor is skipped, not cleared.
|
||||
current = compatibility.parse_semver(core_version)
|
||||
if current is None or current < compatibility.TRUSTWORTHY_FLOOR:
|
||||
self.logger.debug(
|
||||
"Skipping version compatibility check for %s: core __version__ "
|
||||
"(%s) is below the ecosystem floor", plugin_id, core_version)
|
||||
return
|
||||
|
||||
from src import __version__ as core_version
|
||||
current = self._parse_semver(core_version)
|
||||
# Anti-spam guard: if the core's own version number is stale (below
|
||||
# the ecosystem floor every shipped plugin declares), comparing would
|
||||
# warn on nearly everything — skip with a debug note instead.
|
||||
if current is None or current < (2, 0, 0):
|
||||
self.logger.debug(
|
||||
"Skipping version compatibility check for %s: core __version__ "
|
||||
"(%s) is below the ecosystem floor", plugin_id, core_version)
|
||||
return
|
||||
if needed > current:
|
||||
self.logger.warning(
|
||||
"Plugin %s declares min LEDMatrix version %s but this core is %s — "
|
||||
"features it relies on may be missing; update the core or expect "
|
||||
"degraded fallbacks", plugin_id, declared, core_version)
|
||||
declared = compatibility.declared_min_version(manifest)
|
||||
self.logger.warning(
|
||||
"Plugin %s declares min LEDMatrix version %s but this core is %s — "
|
||||
"features it relies on may be missing; update the core or expect "
|
||||
"degraded fallbacks", plugin_id, declared, core_version)
|
||||
|
||||
def load_plugin(
|
||||
self,
|
||||
|
||||
@@ -149,18 +149,27 @@ class PluginStoreManager:
|
||||
# loser can end up renaming the winner's in-progress install aside
|
||||
# mid-download, stealing its own rollback safety net. Keyed by
|
||||
# plugin_id so unrelated plugins still update concurrently.
|
||||
self._reinstall_locks: Dict[str, threading.Lock] = {}
|
||||
# Reentrant: install_plugin takes this lock, and _reinstall_with_rollback
|
||||
# holds it across its call to install_plugin. A plain Lock would
|
||||
# self-deadlock on that nesting.
|
||||
self._reinstall_locks: Dict[str, "threading.RLock"] = {}
|
||||
self._reinstall_locks_guard = threading.Lock()
|
||||
|
||||
# Ensure plugins directory exists
|
||||
self.plugins_dir.mkdir(exist_ok=True)
|
||||
|
||||
def _get_reinstall_lock(self, plugin_id: str) -> threading.Lock:
|
||||
"""Lazily create (or fetch) the per-plugin reinstall lock."""
|
||||
def _get_reinstall_lock(self, plugin_id: str):
|
||||
"""Lazily create (or fetch) the per-plugin reinstall lock.
|
||||
|
||||
Reentrant by necessity: `install_plugin` acquires it to protect its
|
||||
set-aside/restore, and `_reinstall_with_rollback` holds it across its
|
||||
own call to `install_plugin`. With a plain `Lock` that nesting
|
||||
deadlocks the request thread.
|
||||
"""
|
||||
with self._reinstall_locks_guard:
|
||||
lock = self._reinstall_locks.get(plugin_id)
|
||||
if lock is None:
|
||||
lock = threading.Lock()
|
||||
lock = threading.RLock()
|
||||
self._reinstall_locks[plugin_id] = lock
|
||||
return lock
|
||||
|
||||
@@ -1192,6 +1201,90 @@ class PluginStoreManager:
|
||||
return next((p for p in plugins if p.get('id') == plugin_id), None)
|
||||
|
||||
def install_plugin(self, plugin_id: str, branch: Optional[str] = None) -> bool:
|
||||
"""Install a plugin, keeping any existing install until the new one is
|
||||
known good.
|
||||
|
||||
`_install_plugin_impl` deletes the existing directory *before*
|
||||
downloading, so every failure after that point — a dropped connection, a
|
||||
malformed manifest, or the compatibility gate refusing the new version —
|
||||
left the user with no plugin at all. `_reinstall_with_rollback` gives the
|
||||
*update* path exactly this protection; a direct install had none, and the
|
||||
compatibility gate added a new way to reach it.
|
||||
|
||||
Pass-through when nothing is installed, and when called from
|
||||
`_reinstall_with_rollback`, which has already moved the old copy aside.
|
||||
|
||||
The aside name embeds '.standalone-backup-' so plugin discovery
|
||||
(`plugin_manager._scan_directory_for_plugins`) skips it even though it
|
||||
still holds a manifest.json.
|
||||
|
||||
Held under the per-plugin reinstall lock for the same reason
|
||||
`_reinstall_with_rollback` is: the web UI runs Flask with
|
||||
threaded=True, so a double-clicked Install button gives two threads the
|
||||
same plugin_id. Interleaved, one thread's restore would delete the
|
||||
other's freshly installed copy. The lock is reentrant because the
|
||||
rollback path already holds it when it calls in here.
|
||||
"""
|
||||
with self._get_reinstall_lock(plugin_id):
|
||||
plugin_path = self.plugins_dir / plugin_id
|
||||
if not plugin_path.exists():
|
||||
return self._install_plugin_impl(plugin_id, branch)
|
||||
|
||||
backup_path = plugin_path.with_name(
|
||||
f"{plugin_path.name}.standalone-backup-preinstall")
|
||||
if backup_path.exists() and not self._safe_remove_directory(backup_path):
|
||||
# Can't stage a safety net. Better to attempt the install than
|
||||
# to refuse outright, which is what callers got before this
|
||||
# existed.
|
||||
self.logger.warning(
|
||||
"Could not clear stale pre-install backup for %s at %s; "
|
||||
"installing without a rollback net", plugin_id, backup_path)
|
||||
return self._install_plugin_impl(plugin_id, branch)
|
||||
|
||||
try:
|
||||
plugin_path.rename(backup_path)
|
||||
except OSError as e:
|
||||
self.logger.warning(
|
||||
"Could not set aside existing install of %s (%s); "
|
||||
"installing without a rollback net", plugin_id, e)
|
||||
return self._install_plugin_impl(plugin_id, branch)
|
||||
|
||||
try:
|
||||
installed = self._install_plugin_impl(plugin_id, branch)
|
||||
except Exception:
|
||||
self._restore_preinstall_backup(plugin_id, plugin_path, backup_path)
|
||||
raise
|
||||
|
||||
if installed:
|
||||
if not self._safe_remove_directory(backup_path):
|
||||
self.logger.warning(
|
||||
"Install of %s succeeded but the previous copy at %s "
|
||||
"could not be removed; it will be cleared on the next "
|
||||
"install", plugin_id, backup_path)
|
||||
return True
|
||||
|
||||
self._restore_preinstall_backup(plugin_id, plugin_path, backup_path)
|
||||
return False
|
||||
|
||||
def _restore_preinstall_backup(
|
||||
self, plugin_id: str, plugin_path: Path, backup_path: Path
|
||||
) -> None:
|
||||
"""Put the previous install back after a failed (re)install."""
|
||||
self.logger.error(
|
||||
"Install of %s failed; restoring the previous version", plugin_id)
|
||||
try:
|
||||
if plugin_path.exists():
|
||||
# Partial download debris from the failed install.
|
||||
self._safe_remove_directory(plugin_path)
|
||||
backup_path.rename(plugin_path)
|
||||
self.logger.info("Restored previous install of %s", plugin_id)
|
||||
except OSError as e:
|
||||
self.logger.error(
|
||||
"CRITICAL: could not restore %s from %s: %s. The previous "
|
||||
"install is preserved there — rename it back manually.",
|
||||
plugin_id, backup_path, e)
|
||||
|
||||
def _install_plugin_impl(self, plugin_id: str, branch: Optional[str] = None) -> bool:
|
||||
"""
|
||||
Install a plugin from the official registry. Always installs the latest commit
|
||||
from the repository's default branch (or specified branch).
|
||||
@@ -1333,6 +1426,26 @@ class PluginStoreManager:
|
||||
self._safe_remove_directory(plugin_path)
|
||||
return False
|
||||
|
||||
# Refuse a plugin that needs a newer core than this one. The
|
||||
# registry carries no compatibility field, so the floor is only
|
||||
# knowable once the files are down — checking here, before
|
||||
# dependency installation, is the earliest possible point.
|
||||
#
|
||||
# Refusing costs the user nothing: on an update this returns
|
||||
# False and _reinstall_with_rollback restores the version they
|
||||
# already had. Allowing it costs them a plugin that raises
|
||||
# ModuleNotFoundError at load and is reported only as one line
|
||||
# in the journal. See docs/SPORTS_UNIFICATION.md (phase B4/B6).
|
||||
from src import __version__ as core_version
|
||||
from src.plugin_system import compatibility
|
||||
|
||||
compatible, reason = compatibility.check(manifest, core_version)
|
||||
if not compatible:
|
||||
self.logger.error(
|
||||
"Refusing to install %s: %s", plugin_id, reason)
|
||||
self._safe_remove_directory(plugin_path)
|
||||
return False
|
||||
|
||||
if 'entry_point' not in manifest:
|
||||
manifest['entry_point'] = 'manager.py'
|
||||
manifest_modified = True
|
||||
|
||||
Reference in New Issue
Block a user