mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-06 19:28:06 +00:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
cbe6240665 | ||
|
|
74814abf62 | ||
|
|
ffd706a454 | ||
|
|
2af41c561b | ||
|
|
5b81cca684 | ||
|
|
d305be6089 | ||
|
|
53af53b4a1 |
@@ -79,4 +79,9 @@ jobs:
|
||||
test/test_sports_scroll.py \
|
||||
test/test_version_consistency.py \
|
||||
test/test_plugin_compatibility_gate.py \
|
||||
test/test_install_preserves_existing.py
|
||||
test/test_install_preserves_existing.py \
|
||||
test/test_core_owned_config_keys.py \
|
||||
test/test_async_plugin_updates.py \
|
||||
test/test_registry_id_resolution.py \
|
||||
test/test_backup_manager.py \
|
||||
test/test_plugin_update_reservation.py
|
||||
|
||||
@@ -357,6 +357,12 @@ sudo bash ./first_time_install.sh
|
||||
|
||||
This single script installs services, dependencies, configures permissions and sudoers, and validates the setup.
|
||||
|
||||
It finishes by asking whether to reboot. If you run it non-interactively — piped, over a script, or with `-y` — there is no one to ask, so **it reboots immediately without prompting**. Pass `--no-reboot-prompt` to install without rebooting:
|
||||
|
||||
```bash
|
||||
sudo bash ./first_time_install.sh -y --no-reboot-prompt
|
||||
```
|
||||
|
||||
</details>
|
||||
|
||||
</details>
|
||||
|
||||
+29
-18
@@ -1483,26 +1483,37 @@ if [ -f "$PROJECT_ROOT_DIR/config/config.json" ]; then
|
||||
fi
|
||||
|
||||
# Set proper permissions for secrets file (restrictive: owner rw, group r)
|
||||
# If service runs as root, set ownership to root so it can read as owner
|
||||
# Otherwise, use ACTUAL_USER and rely on group membership
|
||||
# Owned by whoever WRITES the file, which is the web interface.
|
||||
#
|
||||
# This used to read the User= of ledmatrix.service — the display service —
|
||||
# and, finding root, hand the file to root:ledmatrix 640. But the display
|
||||
# service only ever reads secrets, and root can read any file regardless of
|
||||
# mode. The account that *writes* them is the web interface: it saves config
|
||||
# edits and performs backup restores, and it deliberately does not run as root
|
||||
# (a web server should not). So a root-owned, group-read-only file left the web
|
||||
# UI unable to write its own secrets, and restoring a backup failed with
|
||||
# "Permission denied: config_secrets.json" while every other file in the same
|
||||
# backup restored fine.
|
||||
#
|
||||
# Owning by the writer keeps the tighter 640 rather than loosening to
|
||||
# group-writable, and root still reads it as superuser.
|
||||
if [ -f "$PROJECT_ROOT_DIR/config/config_secrets.json" ]; then
|
||||
# Check if service runs as root (from service file or template)
|
||||
SERVICE_USER="root"
|
||||
if [ -f "/etc/systemd/system/ledmatrix.service" ]; then
|
||||
SERVICE_USER=$(grep "^User=" /etc/systemd/system/ledmatrix.service | cut -d'=' -f2 || echo "root")
|
||||
elif [ -f "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" ]; then
|
||||
SERVICE_USER=$(grep "^User=" "$PROJECT_ROOT_DIR/systemd/ledmatrix.service" | cut -d'=' -f2 || echo "root")
|
||||
fi
|
||||
|
||||
if [ "$SERVICE_USER" = "root" ]; then
|
||||
# Service runs as root - set ownership to root so it can read as owner
|
||||
chown "root:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true
|
||||
echo "✓ Secrets file permissions set (root:ledmatrix for root service)"
|
||||
else
|
||||
# Service runs as regular user - use ACTUAL_USER and rely on group membership
|
||||
chown "$ACTUAL_USER:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true
|
||||
echo "✓ Secrets file permissions set ($ACTUAL_USER:ledmatrix)"
|
||||
# The web service is the writer; fall back to the display service, then to
|
||||
# the installing user, so an unusual layout still lands somewhere sensible.
|
||||
SECRETS_OWNER=""
|
||||
for unit in "/etc/systemd/system/ledmatrix-web.service" \
|
||||
"$PROJECT_ROOT_DIR/systemd/ledmatrix-web.service"; do
|
||||
if [ -f "$unit" ]; then
|
||||
SECRETS_OWNER=$(grep -m1 "^User=" "$unit" | cut -d'=' -f2)
|
||||
[ -n "$SECRETS_OWNER" ] && break
|
||||
fi
|
||||
done
|
||||
if [ -z "$SECRETS_OWNER" ]; then
|
||||
SECRETS_OWNER="$ACTUAL_USER"
|
||||
fi
|
||||
# A root-owned file is only correct when the writer really is root.
|
||||
chown "$SECRETS_OWNER:$LEDMATRIX_GROUP" "$PROJECT_ROOT_DIR/config/config_secrets.json" || true
|
||||
echo "✓ Secrets file owned by the web service user ($SECRETS_OWNER:$LEDMATRIX_GROUP, mode 640)"
|
||||
chmod 640 "$PROJECT_ROOT_DIR/config/config_secrets.json"
|
||||
fi
|
||||
|
||||
|
||||
+77
-1
@@ -16,6 +16,7 @@ import json
|
||||
import logging
|
||||
import os
|
||||
import shutil
|
||||
import stat
|
||||
import socket
|
||||
import tempfile
|
||||
import zipfile
|
||||
@@ -82,6 +83,10 @@ BUNDLED_FONTS: frozenset[str] = frozenset({
|
||||
_CONFIG_REL = Path("config/config.json")
|
||||
_SECRETS_REL = Path("config/config_secrets.json")
|
||||
_WIFI_REL = Path("config/wifi_config.json")
|
||||
# Sits in config/ next to the three above and is pure user state — a
|
||||
# YouTube Music session that has to be re-authenticated by hand if lost.
|
||||
# It was omitted from backups, so a restore silently signed the user out.
|
||||
_YTM_REL = Path("config/ytm_auth.json")
|
||||
_FONTS_REL = Path("assets/fonts")
|
||||
_PLUGIN_UPLOADS_REL = Path("assets/plugins")
|
||||
_STATE_REL = Path("data/plugin_state.json")
|
||||
@@ -303,6 +308,9 @@ def create_backup(
|
||||
if (project_root / _WIFI_REL).exists():
|
||||
zf.write(project_root / _WIFI_REL, _WIFI_REL.as_posix())
|
||||
contents.append("wifi")
|
||||
if (project_root / _YTM_REL).exists():
|
||||
zf.write(project_root / _YTM_REL, _YTM_REL.as_posix())
|
||||
contents.append("ytm_auth")
|
||||
|
||||
# User-uploaded fonts.
|
||||
user_fonts = iter_user_fonts(project_root)
|
||||
@@ -348,6 +356,7 @@ def preview_backup_contents(project_root: Path) -> Dict[str, Any]:
|
||||
"has_config": (project_root / _CONFIG_REL).exists(),
|
||||
"has_secrets": (project_root / _SECRETS_REL).exists(),
|
||||
"has_wifi": (project_root / _WIFI_REL).exists(),
|
||||
"has_ytm_auth": (project_root / _YTM_REL).exists(),
|
||||
"user_fonts": [p.name for p in iter_user_fonts(project_root)],
|
||||
"plugin_uploads": len(iter_plugin_uploads(project_root)),
|
||||
"plugins": list_installed_plugins(project_root),
|
||||
@@ -429,6 +438,8 @@ def validate_backup(zip_path: Path) -> Tuple[bool, str, Dict[str, Any]]:
|
||||
detected.append("secrets")
|
||||
if _WIFI_REL.as_posix() in names:
|
||||
detected.append("wifi")
|
||||
if _YTM_REL.as_posix() in names:
|
||||
detected.append("ytm_auth")
|
||||
if any(n.startswith(_FONTS_REL.as_posix() + "/") for n in names):
|
||||
detected.append("fonts")
|
||||
if any(
|
||||
@@ -481,8 +492,61 @@ def _extract_zip_safe(zip_path: Path, dest_dir: Path) -> None:
|
||||
|
||||
|
||||
def _copy_file(src: Path, dst: Path) -> None:
|
||||
"""Replace ``dst`` with ``src``, atomically, without needing to own ``dst``.
|
||||
|
||||
``shutil.copy2`` opens the destination for writing, so it needs write
|
||||
permission on the *existing file*. Several config files are installed
|
||||
root-owned and group-readable while the web interface — which is what runs
|
||||
a restore — deliberately runs as a non-root user. Restoring those failed
|
||||
with EACCES even though the account could create files in the same
|
||||
directory perfectly well.
|
||||
|
||||
Writing a temporary file alongside and renaming over the target needs only
|
||||
directory permission, which the web user has. It is also atomic: a crash
|
||||
mid-restore can no longer leave a half-written config behind.
|
||||
|
||||
The destination's existing mode is preserved when there is one, so
|
||||
restoring secrets does not silently widen them to the umask default.
|
||||
"""
|
||||
dst.parent.mkdir(parents=True, exist_ok=True)
|
||||
shutil.copy2(src, dst)
|
||||
|
||||
existing_mode: Optional[int] = None
|
||||
existing_owner: Optional[Tuple[int, int]] = None
|
||||
if dst.exists():
|
||||
try:
|
||||
info = dst.stat()
|
||||
existing_mode = stat.S_IMODE(info.st_mode)
|
||||
existing_owner = (info.st_uid, info.st_gid)
|
||||
except OSError:
|
||||
existing_mode = None
|
||||
existing_owner = None
|
||||
|
||||
fd, tmp_name = tempfile.mkstemp(dir=str(dst.parent), prefix=f".{dst.name}.", suffix=".tmp")
|
||||
os.close(fd)
|
||||
tmp_path = Path(tmp_name)
|
||||
try:
|
||||
shutil.copyfile(src, tmp_path)
|
||||
if existing_mode is not None:
|
||||
os.chmod(tmp_path, existing_mode)
|
||||
else:
|
||||
shutil.copymode(src, tmp_path)
|
||||
if existing_owner is not None:
|
||||
# Replacing a file creates a new inode owned by whoever is running,
|
||||
# which would silently move a root-owned config to the web user.
|
||||
# Carry the previous owner across when the OS permits it — only
|
||||
# root can hand a file to another user, so this is best-effort and
|
||||
# a plain restore as the web user simply keeps its own ownership.
|
||||
try:
|
||||
os.chown(tmp_path, existing_owner[0], existing_owner[1])
|
||||
except (OSError, PermissionError):
|
||||
pass
|
||||
os.replace(tmp_path, dst)
|
||||
except BaseException:
|
||||
try:
|
||||
tmp_path.unlink()
|
||||
except OSError:
|
||||
pass
|
||||
raise
|
||||
|
||||
|
||||
def restore_backup(
|
||||
@@ -546,6 +610,18 @@ def restore_backup(
|
||||
elif (tmp_dir / _WIFI_REL).exists():
|
||||
result.skipped.append("wifi")
|
||||
|
||||
# YouTube Music session. Follows restore_wifi rather than getting its
|
||||
# own flag: it is device-local auth in the same sense, and a separate
|
||||
# toggle for one file would be noise in the restore dialog.
|
||||
if options.restore_wifi and (tmp_dir / _YTM_REL).exists():
|
||||
try:
|
||||
_copy_file(tmp_dir / _YTM_REL, project_root / _YTM_REL)
|
||||
result.restored.append("ytm_auth")
|
||||
except OSError as e:
|
||||
result.errors.append(f"Failed to restore ytm_auth.json: {e}")
|
||||
elif (tmp_dir / _YTM_REL).exists():
|
||||
result.skipped.append("ytm_auth")
|
||||
|
||||
# User fonts — skip anything that collides with a bundled font.
|
||||
tmp_fonts = tmp_dir / _FONTS_REL
|
||||
if options.restore_fonts and tmp_fonts.exists():
|
||||
|
||||
@@ -44,9 +44,16 @@ class DataSource(ABC):
|
||||
"""Fetch standings for a sport/league."""
|
||||
|
||||
def get_headers(self) -> Dict[str, str]:
|
||||
"""Get headers for API requests."""
|
||||
"""Get headers for API requests.
|
||||
|
||||
The agent carries the project URL deliberately. Around 2026-08-04 ESPN
|
||||
began returning 403 for bare custom tokens like 'LEDMatrix/1.0' — and
|
||||
for browser-style strings — while accepting an agent that identifies
|
||||
the client and links to it. An Accept header alone does not rescue the
|
||||
bare form when the request goes out through requests.
|
||||
"""
|
||||
return {
|
||||
'User-Agent': 'LEDMatrix/1.0',
|
||||
'User-Agent': 'LEDMatrix/1.0 (+https://github.com/ChuckBuilds/LEDMatrix)',
|
||||
'Accept': 'application/json'
|
||||
}
|
||||
|
||||
|
||||
@@ -56,7 +56,9 @@ class APIHelper:
|
||||
|
||||
# Default headers
|
||||
self.session.headers.update({
|
||||
'User-Agent': 'LEDMatrix-Common/1.0',
|
||||
# Identifies the client and links to it: ESPN began 403ing bare
|
||||
# custom tokens (and browser strings) around 2026-08-04.
|
||||
'User-Agent': 'LEDMatrix/1.0 (+https://github.com/ChuckBuilds/LEDMatrix)',
|
||||
'Accept': 'application/json',
|
||||
'Accept-Language': 'en-US,en;q=0.9',
|
||||
'Accept-Encoding': 'gzip, deflate, br',
|
||||
|
||||
@@ -113,6 +113,15 @@ class PluginManager:
|
||||
self._update_queue: "queue.Queue[Optional[Tuple[str, float]]]" = queue.Queue()
|
||||
self._pending_updates: set = set()
|
||||
self._pending_lock = threading.Lock()
|
||||
# Serializes the "is this plugin eligible?" -> "claim it (RUNNING)"
|
||||
# transition. Two schedulers run concurrently in practice — the render
|
||||
# loop's _tick_plugin_updates() and Vegas mode's vegas-plugin-tick
|
||||
# daemon thread, which is never joined — so without this both can
|
||||
# observe ENABLED and both call update() on the same plugin. Held only
|
||||
# across the check and the state transition, never across update()
|
||||
# itself: that would serialize slow plugins behind each other and
|
||||
# reintroduce the stall the async worker exists to avoid.
|
||||
self._reservation_lock = threading.Lock()
|
||||
self._plugin_locks: Dict[str, threading.Lock] = {}
|
||||
self._plugin_locks_guard = threading.Lock()
|
||||
self._update_worker: Optional[threading.Thread] = None
|
||||
@@ -395,6 +404,37 @@ class PluginManager:
|
||||
self.state_manager.set_state(plugin_id, PluginState.ERROR, error=e)
|
||||
return False
|
||||
|
||||
#: Config keys the **core** reads out of a plugin's own config block. The
|
||||
#: plugin never declares them, so a schema with
|
||||
#: ``"additionalProperties": false`` — 37 of the 42 published ones — reports
|
||||
#: them as violations and the plugin gets flagged degraded in the web UI for
|
||||
#: using a documented core feature.
|
||||
#:
|
||||
#: Listed explicitly rather than matched on a ``vegas_`` prefix, because
|
||||
#: ``vegas_mode`` is the opposite case: plugins *do* declare that one, and a
|
||||
#: prefix rule would silently stop validating it.
|
||||
#:
|
||||
#: Read by: ``vegas_mode/plugin_adapter.py`` (``vegas_width_pct``,
|
||||
#: ``vegas_overflow``) and ``base_plugin.py`` (``vegas_max_width_screens``).
|
||||
CORE_OWNED_CONFIG_KEYS = frozenset({
|
||||
'vegas_width_pct',
|
||||
'vegas_overflow',
|
||||
'vegas_max_width_screens',
|
||||
})
|
||||
|
||||
def _strip_core_owned_keys(self, config: Dict[str, Any]) -> Dict[str, Any]:
|
||||
"""A shallow copy of ``config`` without the core's own tuning keys.
|
||||
|
||||
Only the top level is touched, and only when such a key is present, so
|
||||
the common case allocates nothing extra.
|
||||
"""
|
||||
if not isinstance(config, dict):
|
||||
return config
|
||||
if not self.CORE_OWNED_CONFIG_KEYS.intersection(config):
|
||||
return config
|
||||
return {k: v for k, v in config.items()
|
||||
if k not in self.CORE_OWNED_CONFIG_KEYS}
|
||||
|
||||
def _validate_config_schema_soft(self, plugin_id: str, config: Dict[str, Any]) -> None:
|
||||
"""Validate a plugin's config against its JSON schema — warn/degrade only.
|
||||
|
||||
@@ -419,7 +459,7 @@ class PluginManager:
|
||||
|
||||
try:
|
||||
is_valid, errors = self.schema_manager.validate_config_against_schema(
|
||||
config, schema, plugin_id
|
||||
self._strip_core_owned_keys(config), schema, plugin_id
|
||||
)
|
||||
except Exception as e: # pragma: no cover - defensive
|
||||
# Validation machinery itself failed — do not penalise the plugin.
|
||||
@@ -777,25 +817,69 @@ class PluginManager:
|
||||
if self.health_tracker and self.health_tracker.should_skip_plugin(plugin_id):
|
||||
continue
|
||||
|
||||
# Check if plugin can execute
|
||||
if not self.state_manager.can_execute(plugin_id):
|
||||
continue
|
||||
|
||||
interval = self._get_plugin_update_interval(plugin_id, plugin_instance)
|
||||
if interval is None:
|
||||
continue
|
||||
|
||||
with self._plugin_last_update_lock:
|
||||
last_update = self.plugin_last_update.get(plugin_id, 0.0)
|
||||
# Eligibility check, due check and the RUNNING transition happen
|
||||
# together, so a concurrent scheduler cannot claim the same plugin.
|
||||
if not self._reserve_for_update(plugin_id, current_time, interval):
|
||||
continue
|
||||
|
||||
if last_update == 0.0 or (current_time - last_update) >= interval:
|
||||
if self._synchronous_updates:
|
||||
# Kill-switch path: the original inline execution
|
||||
# (blocks the caller until update() completes/times out)
|
||||
self.state_manager.set_state(plugin_id, PluginState.RUNNING)
|
||||
self._execute_update_now(plugin_id, plugin_instance, current_time)
|
||||
else:
|
||||
self._enqueue_update(plugin_id, current_time)
|
||||
if self._synchronous_updates:
|
||||
# Kill-switch path: the original inline execution
|
||||
# (blocks the caller until update() completes/times out)
|
||||
self._execute_update_now(plugin_id, plugin_instance, current_time)
|
||||
else:
|
||||
self._enqueue_update(plugin_id, current_time)
|
||||
|
||||
def _reserve_for_update(
|
||||
self,
|
||||
plugin_id: str,
|
||||
current_time: Optional[float] = None,
|
||||
interval: Optional[float] = None,
|
||||
) -> bool:
|
||||
"""Atomically claim a plugin for update, returning True if we won it.
|
||||
|
||||
can_execute() and the RUNNING transition have to happen under one lock.
|
||||
As two separate calls, two scheduler threads can both see ENABLED and
|
||||
both go on to run the same plugin's update() concurrently — unsafe for
|
||||
any plugin that isn't reentrant (shared mutable state, a non-thread-safe
|
||||
HTTP session or cache).
|
||||
|
||||
The due-time check is inside the lock too. Leaving it outside would let
|
||||
a second thread that had already decided "due" claim the plugin the
|
||||
instant the first finished, running update() twice in one interval.
|
||||
|
||||
Args:
|
||||
plugin_id: Plugin to claim.
|
||||
current_time: Now, for the due check. Omit to skip that check.
|
||||
interval: Seconds between updates. Omit to skip the due check.
|
||||
|
||||
Returns:
|
||||
True if this caller reserved the plugin and must dispatch it,
|
||||
False if it is ineligible, not yet due, or already claimed.
|
||||
"""
|
||||
with self._reservation_lock:
|
||||
if not self.state_manager.can_execute(plugin_id):
|
||||
return False
|
||||
|
||||
if current_time is not None and interval is not None:
|
||||
with self._plugin_last_update_lock:
|
||||
last_update = self.plugin_last_update.get(plugin_id, 0.0)
|
||||
if last_update != 0.0 and (current_time - last_update) < interval:
|
||||
return False
|
||||
|
||||
self.state_manager.set_state(plugin_id, PluginState.RUNNING)
|
||||
return True
|
||||
|
||||
def _release_reservation(self, plugin_id: str) -> None:
|
||||
"""Hand a claimed plugin back when it never got dispatched.
|
||||
|
||||
Without this a plugin reserved but not queued would sit in RUNNING
|
||||
forever, and can_execute() would refuse it on every later tick.
|
||||
"""
|
||||
self.state_manager.set_state(plugin_id, PluginState.ENABLED)
|
||||
|
||||
def get_plugin_lock(self, plugin_id: str) -> threading.Lock:
|
||||
"""Per-plugin lock keeping update() and display() mutually exclusive.
|
||||
@@ -812,16 +896,40 @@ class PluginManager:
|
||||
return lock
|
||||
|
||||
def _enqueue_update(self, plugin_id: str, scheduled_time: float) -> None:
|
||||
"""Queue a due update for the background worker (dedup while pending)."""
|
||||
"""Queue an already-reserved update for the background worker.
|
||||
|
||||
The caller has reserved the plugin (RUNNING), which is what blocks
|
||||
re-entry and shows the truthful state in the web UI while the item
|
||||
waits its turn. The pending set stays as a second line of defence; if
|
||||
it ever fires the reservation has to be handed back, or the plugin
|
||||
would sit in RUNNING with nothing queued to release it.
|
||||
"""
|
||||
with self._pending_lock:
|
||||
if plugin_id in self._pending_updates:
|
||||
self.logger.warning(
|
||||
"Plugin %s reserved for update but already queued; "
|
||||
"releasing the reservation", plugin_id)
|
||||
self._release_reservation(plugin_id)
|
||||
return
|
||||
self._pending_updates.add(plugin_id)
|
||||
# RUNNING is set at enqueue time so can_execute() blocks re-entry and
|
||||
# the web UI shows the truthful state while the item waits its turn.
|
||||
self.state_manager.set_state(plugin_id, PluginState.RUNNING)
|
||||
self._ensure_update_worker()
|
||||
self._update_queue.put((plugin_id, scheduled_time))
|
||||
try:
|
||||
self._ensure_update_worker()
|
||||
self._update_queue.put((plugin_id, scheduled_time))
|
||||
except Exception as exc: # pylint: disable=broad-except
|
||||
# Thread.start() raises RuntimeError when the OS refuses a new
|
||||
# thread — a real condition on a Pi under memory pressure. Nothing
|
||||
# is queued to release the plugin at that point, so the claim has to
|
||||
# be undone here, or it sits in RUNNING with nothing to clear it and
|
||||
# can_execute() refuses it for the rest of the process. Swallowed
|
||||
# rather than raised so the remaining plugins in this tick still get
|
||||
# their turn.
|
||||
self.logger.error(
|
||||
"Could not queue update for plugin %s (%s: %s); releasing the "
|
||||
"reservation so the next tick can retry",
|
||||
plugin_id, type(exc).__name__, exc, exc_info=True)
|
||||
with self._pending_lock:
|
||||
self._pending_updates.discard(plugin_id)
|
||||
self._release_reservation(plugin_id)
|
||||
|
||||
def _ensure_update_worker(self) -> None:
|
||||
if self._update_worker is not None and self._update_worker.is_alive():
|
||||
@@ -906,6 +1014,14 @@ class PluginManager:
|
||||
return
|
||||
finished['done'] = True
|
||||
try:
|
||||
# Drop the queue reservation *before* the state goes back to
|
||||
# ENABLED. The other order leaves a window where a scheduler
|
||||
# sees ENABLED, reserves the plugin, then finds it still in
|
||||
# _pending_updates -- the enqueue is dropped and the plugin
|
||||
# would sit in RUNNING with nothing left to release it.
|
||||
if lock is not None:
|
||||
with self._pending_lock:
|
||||
self._pending_updates.discard(plugin_id)
|
||||
if success:
|
||||
with self._plugin_last_update_lock:
|
||||
self.plugin_last_update[plugin_id] = scheduled_time
|
||||
@@ -918,8 +1034,6 @@ class PluginManager:
|
||||
finally:
|
||||
if lock is not None:
|
||||
lock.release()
|
||||
with self._pending_lock:
|
||||
self._pending_updates.discard(plugin_id)
|
||||
|
||||
if lock is None:
|
||||
# Synchronous / no-lock path: unchanged behavior.
|
||||
@@ -1010,13 +1124,12 @@ class PluginManager:
|
||||
if not hasattr(plugin_instance, "update"):
|
||||
continue
|
||||
|
||||
# Check if plugin can execute
|
||||
if not self.state_manager.can_execute(plugin_id):
|
||||
# Eligibility check and the RUNNING transition together, so a
|
||||
# concurrent scheduler cannot claim the same plugin (see
|
||||
# _reserve_for_update).
|
||||
if not self._reserve_for_update(plugin_id):
|
||||
continue
|
||||
|
||||
# Update state to RUNNING
|
||||
self.state_manager.set_state(plugin_id, PluginState.RUNNING)
|
||||
|
||||
|
||||
try:
|
||||
success = self.plugin_executor.execute_update(plugin_instance, plugin_id)
|
||||
if success:
|
||||
|
||||
@@ -1143,7 +1143,7 @@ class PluginStoreManager:
|
||||
"""
|
||||
registry = self.fetch_registry()
|
||||
plugins = registry.get('plugins', []) or []
|
||||
plugin_info = next((p for p in plugins if p['id'] == plugin_id), None)
|
||||
plugin_info = self._match_registry_entry(plugins, plugin_id)
|
||||
|
||||
if not plugin_info:
|
||||
return None
|
||||
@@ -1183,6 +1183,37 @@ class PluginStoreManager:
|
||||
|
||||
return plugin_info
|
||||
|
||||
@staticmethod
|
||||
def _match_registry_entry(plugins: List[Dict], plugin_id: str) -> Optional[Dict]:
|
||||
"""Find a registry entry by its id, or by the directory it installs to.
|
||||
|
||||
Four shipped plugins have a registry ``id`` that differs from the ``id``
|
||||
in their own manifest: ``weather`` installs to ``plugins/ledmatrix-weather``,
|
||||
and likewise stocks, music and leaderboard. Installation already prefers
|
||||
the manifest id for the directory name, so on disk, in ``config.json``
|
||||
and in a backup manifest those plugins are called ``ledmatrix-weather``.
|
||||
|
||||
Only the registry calls them ``weather``, and nothing resolved that in
|
||||
reverse: restoring a backup asked the store for ``ledmatrix-weather``
|
||||
and got "Plugin not found in registry", silently dropping four enabled
|
||||
plugins from a restored device.
|
||||
|
||||
Matching ``plugin_path`` fixes it without renaming any published id,
|
||||
which would orphan ``plugin_state.json`` entries keyed on the old ones.
|
||||
Exact id always wins, so an entry whose *path* happens to collide with
|
||||
another entry's id cannot shadow it.
|
||||
"""
|
||||
if not plugin_id:
|
||||
return None
|
||||
exact = next((p for p in plugins if p.get('id') == plugin_id), None)
|
||||
if exact is not None:
|
||||
return exact
|
||||
for entry in plugins:
|
||||
path = (entry.get('plugin_path') or '').rstrip('/')
|
||||
if path and path.rsplit('/', 1)[-1] == plugin_id:
|
||||
return entry
|
||||
return None
|
||||
|
||||
def get_registry_info(self, plugin_id: str) -> Optional[Dict]:
|
||||
"""
|
||||
Get plugin information from the registry cache only (no GitHub API calls).
|
||||
@@ -1198,7 +1229,7 @@ class PluginStoreManager:
|
||||
"""
|
||||
registry = self.fetch_registry()
|
||||
plugins = registry.get('plugins', []) or []
|
||||
return next((p for p in plugins if p.get('id') == plugin_id), None)
|
||||
return self._match_registry_entry(plugins, plugin_id)
|
||||
|
||||
def install_plugin(self, plugin_id: str, branch: Optional[str] = None) -> bool:
|
||||
"""Install a plugin, keeping any existing install until the new one is
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import stat
|
||||
import zipfile
|
||||
from pathlib import Path
|
||||
|
||||
@@ -41,6 +42,13 @@ def _make_project(root: Path) -> Path:
|
||||
json.dumps({"ap_mode": {"ssid": "LEDMatrix"}}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
# Device-local auth that lives in config/ like the three above. It was
|
||||
# omitted from backups, so a restore silently signed the user out of
|
||||
# YouTube Music and they had to re-authenticate by hand.
|
||||
(root / "config" / "ytm_auth.json").write_text(
|
||||
json.dumps({"token": "YTM-TOKEN"}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
fonts = root / "assets" / "fonts"
|
||||
fonts.mkdir(parents=True)
|
||||
@@ -240,6 +248,10 @@ def test_restore_roundtrip(project: Path, empty_project: Path, tmp_path: Path) -
|
||||
restored_secrets = json.loads((empty_project / "config" / "config_secrets.json").read_text())
|
||||
assert restored_secrets["ledmatrix-weather"]["api_key"] == "SECRET"
|
||||
|
||||
assert "ytm_auth" in result.restored
|
||||
restored_ytm = json.loads((empty_project / "config" / "ytm_auth.json").read_text())
|
||||
assert restored_ytm["token"] == "YTM-TOKEN"
|
||||
|
||||
# User font restored, bundled font untouched.
|
||||
assert (empty_project / "assets" / "fonts" / "my-custom-font.ttf").read_bytes() == b"\x00\x01USER"
|
||||
assert (empty_project / "assets" / "fonts" / "5x7.bdf").read_text() == "BUNDLED"
|
||||
@@ -282,3 +294,39 @@ def test_restore_rejects_malicious_zip(empty_project: Path, tmp_path: Path) -> N
|
||||
# validate_backup catches it before extraction.
|
||||
assert not result.success
|
||||
assert any("unsafe" in e.lower() for e in result.errors)
|
||||
|
||||
|
||||
def test_restore_over_a_file_the_user_cannot_write(
|
||||
project: Path, empty_project: Path, tmp_path: Path
|
||||
) -> None:
|
||||
"""Restore must not need write permission on the destination *file*.
|
||||
|
||||
Reproduces what a fresh install leaves behind: config files owned by root
|
||||
and only group-readable, while the web interface that performs the restore
|
||||
runs as a non-root user. shutil.copy2 opens the destination for writing and
|
||||
failed with EACCES; writing alongside and renaming needs only directory
|
||||
permission, which that account has.
|
||||
|
||||
Simulated here by making the destination read-only — the owner cannot
|
||||
open it for writing either, but can still replace it within its directory.
|
||||
"""
|
||||
zip_path = create_backup(project, output_dir=tmp_path / "exports")
|
||||
|
||||
# Pre-existing, read-only destinations.
|
||||
(empty_project / "config").mkdir(parents=True, exist_ok=True)
|
||||
for name in ("config.json", "config_secrets.json", "wifi_config.json", "ytm_auth.json"):
|
||||
target = empty_project / "config" / name
|
||||
target.write_text("{}", encoding="utf-8")
|
||||
target.chmod(0o444)
|
||||
|
||||
result = restore_backup(zip_path, empty_project, RestoreOptions())
|
||||
|
||||
assert result.success, result.errors
|
||||
for section in ("config", "secrets", "wifi", "ytm_auth"):
|
||||
assert section in result.restored, f"{section} not restored: {result.errors}"
|
||||
|
||||
restored = json.loads((empty_project / "config" / "config.json").read_text())
|
||||
assert restored["my-plugin"]["favorites"] == ["A", "B"]
|
||||
|
||||
# The destination's mode is preserved rather than widened to the umask.
|
||||
assert stat.S_IMODE((empty_project / "config" / "config_secrets.json").stat().st_mode) == 0o444
|
||||
|
||||
@@ -0,0 +1,98 @@
|
||||
"""The core's own tuning keys must not make a plugin look broken.
|
||||
|
||||
`vegas_width_pct`, `vegas_overflow` and `vegas_max_width_screens` are read by
|
||||
the *core* out of each plugin's config block — `vegas_mode/plugin_adapter.py`
|
||||
and `base_plugin.py`. No plugin declares them, and 37 of the 42 published
|
||||
config schemas set `"additionalProperties": false`, so schema validation
|
||||
reported them as violations.
|
||||
|
||||
That is not just log noise: `_validate_config_schema_soft` sets `degraded` in
|
||||
the health tracker, which the web UI surfaces. Measured on a real device, **9
|
||||
of 27 installed plugins** were flagged degraded purely for using a documented
|
||||
core feature — including `baseball-scoreboard` and `f1-scoreboard`.
|
||||
|
||||
The fix strips those keys before validating. It deliberately does *not* match
|
||||
on a `vegas_` prefix: `vegas_mode` is plugin-owned and declared in schemas, and
|
||||
a prefix rule would silently stop validating it.
|
||||
"""
|
||||
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
from src.plugin_system.plugin_manager import PluginManager
|
||||
|
||||
|
||||
STRICT_SCHEMA = {
|
||||
"type": "object",
|
||||
"additionalProperties": False,
|
||||
"properties": {
|
||||
"enabled": {"type": "boolean"},
|
||||
"vegas_mode": {"type": "string"}, # plugin-owned, must stay validated
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def manager():
|
||||
mgr = PluginManager.__new__(PluginManager) # skip the heavy constructor
|
||||
mgr.logger = MagicMock()
|
||||
mgr.schema_manager = MagicMock()
|
||||
mgr._set_degraded_safe = MagicMock()
|
||||
return mgr
|
||||
|
||||
|
||||
class TestStripCoreOwnedKeys:
|
||||
def test_removes_every_core_owned_key(self, manager):
|
||||
cfg = {"enabled": True, "vegas_width_pct": 50,
|
||||
"vegas_overflow": "wrap", "vegas_max_width_screens": 2}
|
||||
assert manager._strip_core_owned_keys(cfg) == {"enabled": True}
|
||||
|
||||
def test_leaves_plugin_owned_vegas_mode_alone(self, manager):
|
||||
"""A prefix rule would have eaten this one."""
|
||||
cfg = {"enabled": True, "vegas_mode": "scroll"}
|
||||
assert manager._strip_core_owned_keys(cfg) == cfg
|
||||
|
||||
def test_returns_the_same_object_when_nothing_to_strip(self, manager):
|
||||
cfg = {"enabled": True}
|
||||
assert manager._strip_core_owned_keys(cfg) is cfg
|
||||
|
||||
def test_does_not_mutate_the_caller_config(self, manager):
|
||||
cfg = {"enabled": True, "vegas_width_pct": 50}
|
||||
manager._strip_core_owned_keys(cfg)
|
||||
assert "vegas_width_pct" in cfg, "the live plugin config was mutated"
|
||||
|
||||
def test_tolerates_a_non_dict(self, manager):
|
||||
assert manager._strip_core_owned_keys(None) is None
|
||||
|
||||
|
||||
class TestSoftValidation:
|
||||
def _validate_with(self, manager, config, valid=True, errors=()):
|
||||
manager.schema_manager.load_schema.return_value = STRICT_SCHEMA
|
||||
manager.schema_manager.validate_config_against_schema.return_value = (
|
||||
valid, list(errors))
|
||||
manager._validate_config_schema_soft("baseball-scoreboard", config)
|
||||
return manager.schema_manager.validate_config_against_schema.call_args
|
||||
|
||||
def test_core_keys_never_reach_the_validator(self, manager):
|
||||
"""The regression: these keys reaching a strict schema is what flagged
|
||||
9 of 27 plugins degraded."""
|
||||
args = self._validate_with(
|
||||
manager, {"enabled": True, "vegas_width_pct": 50})
|
||||
validated = args[0][0]
|
||||
assert "vegas_width_pct" not in validated
|
||||
assert validated == {"enabled": True}
|
||||
|
||||
def test_plugin_owned_keys_still_reach_the_validator(self, manager):
|
||||
args = self._validate_with(
|
||||
manager, {"enabled": True, "vegas_mode": "scroll"})
|
||||
assert args[0][0]["vegas_mode"] == "scroll"
|
||||
|
||||
def test_a_genuine_violation_is_still_reported(self, manager):
|
||||
"""Stripping core keys must not turn the check into a no-op."""
|
||||
self._validate_with(
|
||||
manager, {"enabled": True, "typo_key": 1},
|
||||
valid=False, errors=["Field root: 'typo_key' was unexpected"])
|
||||
manager._set_degraded_safe.assert_called()
|
||||
reason = manager._set_degraded_safe.call_args[0][1]
|
||||
assert reason and "typo_key" in reason
|
||||
@@ -0,0 +1,335 @@
|
||||
"""Plugin update scheduling must be atomic (issue #401).
|
||||
|
||||
`run_scheduled_updates()` decided whether to update a plugin with a
|
||||
check-then-act sequence: `can_execute()` and `set_state(RUNNING)` were separate
|
||||
calls with nothing between them, so two scheduler threads could both observe
|
||||
ENABLED and both go on to call the same plugin's `update()`.
|
||||
|
||||
Two schedulers really do run at once. The render loop calls
|
||||
`_tick_plugin_updates()`, and Vegas mode fires its own `vegas-plugin-tick`
|
||||
daemon thread every few seconds; that thread is never joined when
|
||||
`VegasModeCoordinator.play()` returns, so a slow `update()` still in flight can
|
||||
overlap the next tick from the main loop.
|
||||
|
||||
A plugin whose `update()` runs twice at once is unsafe unless it happens to be
|
||||
reentrant — shared mutable state, a non-thread-safe HTTP session or cache all
|
||||
break. These tests pin the reservation that closes it, and the state
|
||||
bookkeeping that has to survive it: a plugin reserved but never dispatched must
|
||||
not be stranded in RUNNING, because `can_execute()` would then refuse it
|
||||
forever.
|
||||
"""
|
||||
|
||||
import os
|
||||
import sys
|
||||
import threading
|
||||
import time
|
||||
|
||||
import pytest
|
||||
|
||||
sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
|
||||
|
||||
from src.plugin_system.plugin_manager import PluginManager # noqa: E402
|
||||
from src.plugin_system.plugin_state import PluginState # noqa: E402
|
||||
|
||||
|
||||
class OverlapDetectingPlugin:
|
||||
"""Records the high-water mark of concurrent update() calls."""
|
||||
|
||||
def __init__(self, update_seconds=0.25):
|
||||
self.enabled = True
|
||||
self.update_seconds = update_seconds
|
||||
self.update_calls = 0
|
||||
self.max_concurrent = 0
|
||||
self._active = 0
|
||||
self._guard = threading.Lock()
|
||||
|
||||
def update(self):
|
||||
with self._guard:
|
||||
self._active += 1
|
||||
self.update_calls += 1
|
||||
self.max_concurrent = max(self.max_concurrent, self._active)
|
||||
try:
|
||||
time.sleep(self.update_seconds)
|
||||
finally:
|
||||
with self._guard:
|
||||
self._active -= 1
|
||||
return True
|
||||
|
||||
def display(self, force_clear=False):
|
||||
return True
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def pm(tmp_path):
|
||||
manager = PluginManager(plugins_dir=str(tmp_path), config_manager=None,
|
||||
display_manager=None, cache_manager=None)
|
||||
yield manager
|
||||
manager.stop_update_worker()
|
||||
|
||||
|
||||
def _install(pm, plugin, plugin_id="racy-plugin", interval=0.01):
|
||||
pm.plugins[plugin_id] = plugin
|
||||
pm._update_interval_cache[plugin_id] = interval
|
||||
pm.state_manager.set_state(plugin_id, PluginState.ENABLED)
|
||||
return plugin_id
|
||||
|
||||
|
||||
def _widen_check_then_act_window(pm, delay=0.02):
|
||||
"""Hold every scheduler thread inside the eligibility check at once.
|
||||
|
||||
The real gap between can_execute() and the RUNNING transition is a couple
|
||||
of bytecodes wide, so a plain thread race under the GIL almost never lands
|
||||
in it — an unfixed scheduler looks correct in a test that just hammers it.
|
||||
Delaying the check reproduces the interleaving that Vegas's tick thread and
|
||||
the render loop actually produce when a slow update() overlaps the next
|
||||
tick, and it is what makes these tests fail without the reservation.
|
||||
|
||||
Once the check and the transition are under one lock, the delay only
|
||||
serializes the schedulers: the losers observe RUNNING and back off.
|
||||
"""
|
||||
real_can_execute = pm.state_manager.can_execute
|
||||
|
||||
def slow_can_execute(plugin_id):
|
||||
result = real_can_execute(plugin_id)
|
||||
time.sleep(delay)
|
||||
return result
|
||||
|
||||
pm.state_manager.can_execute = slow_can_execute
|
||||
|
||||
|
||||
def _hammer(target, threads=8, rounds=1):
|
||||
"""Run `target` on N threads released simultaneously by a barrier."""
|
||||
errors = []
|
||||
barrier = threading.Barrier(threads)
|
||||
|
||||
def runner():
|
||||
try:
|
||||
barrier.wait(timeout=5)
|
||||
for _ in range(rounds):
|
||||
target()
|
||||
except Exception as exc: # noqa: BLE001 - surfaced by the assertion
|
||||
errors.append(exc)
|
||||
|
||||
workers = [threading.Thread(target=runner) for _ in range(threads)]
|
||||
for worker in workers:
|
||||
worker.start()
|
||||
for worker in workers:
|
||||
worker.join(timeout=30)
|
||||
assert not errors, f"worker raised: {errors[0]!r}"
|
||||
|
||||
|
||||
class TestReservationAtomicity:
|
||||
"""The check-and-claim itself, independent of any dispatch path."""
|
||||
|
||||
def test_only_one_caller_wins_the_reservation(self, pm):
|
||||
plugin_id = _install(pm, OverlapDetectingPlugin())
|
||||
_widen_check_then_act_window(pm)
|
||||
wins = []
|
||||
lock = threading.Lock()
|
||||
|
||||
def claim():
|
||||
if pm._reserve_for_update(plugin_id):
|
||||
with lock:
|
||||
wins.append(threading.current_thread().name)
|
||||
|
||||
_hammer(claim, threads=16)
|
||||
assert len(wins) == 1, (
|
||||
f"{len(wins)} threads reserved the same plugin concurrently; "
|
||||
"the eligibility check and the RUNNING transition are not atomic")
|
||||
assert pm.state_manager.get_state(plugin_id) == PluginState.RUNNING
|
||||
|
||||
def test_reservation_refused_while_running(self, pm):
|
||||
plugin_id = _install(pm, OverlapDetectingPlugin())
|
||||
assert pm._reserve_for_update(plugin_id) is True
|
||||
assert pm._reserve_for_update(plugin_id) is False
|
||||
|
||||
def test_reservation_can_be_handed_back(self, pm):
|
||||
plugin_id = _install(pm, OverlapDetectingPlugin())
|
||||
assert pm._reserve_for_update(plugin_id) is True
|
||||
pm._release_reservation(plugin_id)
|
||||
assert pm.state_manager.get_state(plugin_id) == PluginState.ENABLED
|
||||
assert pm._reserve_for_update(plugin_id) is True, \
|
||||
"a released reservation must be claimable again"
|
||||
|
||||
def test_due_check_is_inside_the_reservation(self, pm):
|
||||
"""Two threads that both decided 'due' must not both get a turn.
|
||||
|
||||
With the due check outside the lock, the loser of the race could claim
|
||||
the plugin the moment the winner finished, running update() twice
|
||||
inside one interval.
|
||||
"""
|
||||
plugin_id = _install(pm, OverlapDetectingPlugin(), interval=60.0)
|
||||
now = time.time()
|
||||
assert pm._reserve_for_update(plugin_id, now, 60.0) is True
|
||||
pm.plugin_last_update[plugin_id] = now
|
||||
pm._release_reservation(plugin_id)
|
||||
assert pm._reserve_for_update(plugin_id, now, 60.0) is False, \
|
||||
"plugin updated just now must not be due again"
|
||||
|
||||
|
||||
class TestNoConcurrentUpdate:
|
||||
"""The end-to-end invariant the issue is actually about."""
|
||||
|
||||
def test_synchronous_path_never_overlaps(self, pm):
|
||||
"""The kill-switch path ran update() inline with no dedup at all."""
|
||||
pm._synchronous_updates = True
|
||||
plugin = OverlapDetectingPlugin(update_seconds=0.25)
|
||||
_install(pm, plugin)
|
||||
_widen_check_then_act_window(pm)
|
||||
|
||||
_hammer(pm.run_scheduled_updates, threads=8)
|
||||
|
||||
assert plugin.max_concurrent == 1, (
|
||||
f"update() ran {plugin.max_concurrent}x concurrently on the "
|
||||
"synchronous path")
|
||||
|
||||
def test_update_all_plugins_never_overlaps(self, pm):
|
||||
plugin = OverlapDetectingPlugin(update_seconds=0.25)
|
||||
_install(pm, plugin)
|
||||
_widen_check_then_act_window(pm)
|
||||
|
||||
_hammer(pm.update_all_plugins, threads=8)
|
||||
|
||||
assert plugin.max_concurrent == 1, (
|
||||
f"update() ran {plugin.max_concurrent}x concurrently via "
|
||||
"update_all_plugins()")
|
||||
|
||||
def test_async_path_never_overlaps(self, pm):
|
||||
plugin = OverlapDetectingPlugin(update_seconds=0.2)
|
||||
plugin_id = _install(pm, plugin)
|
||||
|
||||
_hammer(pm.run_scheduled_updates, threads=8, rounds=3)
|
||||
|
||||
# Wait for an update to have both started and finished. Polling only
|
||||
# `_active` races the worker: before it picks the item up nothing is
|
||||
# active yet, so the loop would fall straight through and assert on a
|
||||
# plugin that never ran.
|
||||
deadline = time.monotonic() + 15
|
||||
while time.monotonic() < deadline:
|
||||
if plugin.update_calls >= 1 and plugin._active == 0:
|
||||
break
|
||||
time.sleep(0.05)
|
||||
|
||||
assert plugin.update_calls >= 1, "no update ran on the async path"
|
||||
assert plugin.max_concurrent == 1, (
|
||||
f"update() ran {plugin.max_concurrent}x concurrently on the "
|
||||
"async path")
|
||||
assert plugin_id in pm.plugins
|
||||
|
||||
|
||||
class TestNoStrandedState:
|
||||
"""A reservation that is never dispatched must not wedge the plugin."""
|
||||
|
||||
def test_plugin_returns_to_enabled_after_async_updates(self, pm):
|
||||
plugin = OverlapDetectingPlugin(update_seconds=0.1)
|
||||
plugin_id = _install(pm, plugin)
|
||||
|
||||
_hammer(pm.run_scheduled_updates, threads=6, rounds=2)
|
||||
|
||||
deadline = time.monotonic() + 15
|
||||
while time.monotonic() < deadline:
|
||||
if (plugin.update_calls >= 1
|
||||
and pm.state_manager.get_state(plugin_id) == PluginState.ENABLED
|
||||
and not pm._pending_updates):
|
||||
break
|
||||
time.sleep(0.05)
|
||||
|
||||
# ENABLED is also the starting state, so without this the assertion
|
||||
# below would pass on a plugin that never got scheduled at all.
|
||||
assert plugin.update_calls >= 1, "no update ran; the state assertion would be vacuous"
|
||||
assert pm.state_manager.get_state(plugin_id) == PluginState.ENABLED, \
|
||||
"plugin stranded in RUNNING; can_execute() would refuse it forever"
|
||||
assert not pm._pending_updates, "pending set not drained"
|
||||
|
||||
def test_pending_cleared_before_state_reset(self, pm):
|
||||
"""Invariant: never-RUNNING implies never-pending.
|
||||
|
||||
_finish() used to clear the pending entry *after* flipping the state
|
||||
back to ENABLED. In that window a scheduler could reserve the plugin
|
||||
and then have its enqueue dropped by the pending-dedup.
|
||||
"""
|
||||
plugin = OverlapDetectingPlugin(update_seconds=0.05)
|
||||
plugin_id = _install(pm, plugin)
|
||||
|
||||
violations = []
|
||||
stop = threading.Event()
|
||||
|
||||
def watcher():
|
||||
while not stop.is_set():
|
||||
state = pm.state_manager.get_state(plugin_id)
|
||||
if state != PluginState.RUNNING and plugin_id in pm._pending_updates:
|
||||
violations.append(state)
|
||||
time.sleep(0.001)
|
||||
|
||||
thread = threading.Thread(target=watcher, daemon=True)
|
||||
thread.start()
|
||||
try:
|
||||
for _ in range(15):
|
||||
pm.run_scheduled_updates()
|
||||
time.sleep(0.05)
|
||||
finally:
|
||||
stop.set()
|
||||
thread.join(timeout=5)
|
||||
|
||||
assert not violations, (
|
||||
f"{len(violations)} sample(s) saw a non-RUNNING plugin still in "
|
||||
"_pending_updates")
|
||||
|
||||
|
||||
class TestDispatchFailure:
|
||||
"""A reservation must survive the dispatch itself failing.
|
||||
|
||||
_enqueue_update() claims the plugin, adds it to the pending set, and only
|
||||
then starts the worker and queues the item. threading.Thread.start() raises
|
||||
RuntimeError when the OS refuses a new thread — not hypothetical on a Pi
|
||||
under memory or thread pressure. Nothing is queued to release the plugin at
|
||||
that point, so without an explicit rollback it stays RUNNING with a stale
|
||||
pending entry and can_execute() refuses it for the rest of the process.
|
||||
"""
|
||||
|
||||
def test_reservation_released_when_the_worker_cannot_start(self, pm):
|
||||
plugin_id = _install(pm, OverlapDetectingPlugin())
|
||||
|
||||
def refuse_to_start():
|
||||
raise RuntimeError("can't start new thread")
|
||||
|
||||
pm._ensure_update_worker = refuse_to_start
|
||||
|
||||
assert pm._reserve_for_update(plugin_id) is True
|
||||
pm._enqueue_update(plugin_id, time.time())
|
||||
|
||||
assert pm.state_manager.get_state(plugin_id) == PluginState.ENABLED, \
|
||||
"plugin left in RUNNING after a failed dispatch; can_execute() " \
|
||||
"would refuse it forever"
|
||||
assert plugin_id not in pm._pending_updates, \
|
||||
"stale pending entry would make the next enqueue hit the dedup"
|
||||
assert pm._reserve_for_update(plugin_id) is True, \
|
||||
"plugin must be claimable again on the next tick"
|
||||
|
||||
def test_dispatch_failure_does_not_abort_the_rest_of_the_tick(self, pm):
|
||||
"""One plugin failing to queue must not skip the others in that tick."""
|
||||
first = OverlapDetectingPlugin()
|
||||
second = OverlapDetectingPlugin()
|
||||
_install(pm, first, plugin_id="plugin-a")
|
||||
_install(pm, second, plugin_id="plugin-b")
|
||||
|
||||
calls = []
|
||||
real_ensure = pm._ensure_update_worker
|
||||
|
||||
def fail_first_only():
|
||||
calls.append(1)
|
||||
if len(calls) == 1:
|
||||
raise RuntimeError("can't start new thread")
|
||||
return real_ensure()
|
||||
|
||||
pm._ensure_update_worker = fail_first_only
|
||||
|
||||
pm.run_scheduled_updates() # must not propagate the RuntimeError
|
||||
|
||||
assert len(calls) == 2, (
|
||||
"run_scheduled_updates() stopped after the failing plugin; the "
|
||||
"exception escaped the enqueue")
|
||||
for plugin_id in ("plugin-a", "plugin-b"):
|
||||
assert pm.state_manager.get_state(plugin_id) in (
|
||||
PluginState.ENABLED, PluginState.RUNNING), \
|
||||
f"{plugin_id} left in an unexpected state"
|
||||
@@ -0,0 +1,107 @@
|
||||
"""A plugin must be findable in the registry by the id it calls itself.
|
||||
|
||||
Four shipped plugins have a registry ``id`` that differs from the ``id`` in
|
||||
their own ``manifest.json``:
|
||||
|
||||
directory / manifest.json id registry id
|
||||
ledmatrix-weather weather
|
||||
ledmatrix-stocks stocks
|
||||
ledmatrix-music music
|
||||
ledmatrix-leaderboard leaderboard
|
||||
|
||||
The installer already knows about this: it deliberately names the install
|
||||
directory after the *manifest* id (store_manager, "Use manifest ID for
|
||||
directory name"), and warns when the two disagree. So on disk, in
|
||||
``config.json`` and in a backup manifest, these plugins are called
|
||||
``ledmatrix-weather``. Only the registry calls them ``weather``.
|
||||
|
||||
Nothing resolved that in reverse. Asking the store to install
|
||||
``ledmatrix-weather`` -- which is exactly what restoring a backup does --
|
||||
failed with "Plugin not found in registry", and four enabled plugins went
|
||||
missing from a restored device with no error surfaced to the user.
|
||||
|
||||
Renaming the registry ids would orphan existing ``plugin_state.json`` entries
|
||||
keyed on the old ones, so the lookup resolves ``plugin_path`` instead: the
|
||||
registry already records ``plugins/ledmatrix-weather``, which is unambiguous
|
||||
and needs no published identity to change.
|
||||
"""
|
||||
|
||||
import os
|
||||
import sys
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
import pytest
|
||||
|
||||
sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
|
||||
|
||||
from src.plugin_system.store_manager import PluginStoreManager # noqa: E402
|
||||
|
||||
|
||||
# Shaped like the real registry: id and plugin_path basename disagree for the
|
||||
# first entry, agree for the second.
|
||||
REGISTRY: Dict[str, List[Dict[str, Any]]] = {
|
||||
"plugins": [
|
||||
{
|
||||
"id": "weather",
|
||||
"name": "Weather",
|
||||
"plugin_path": "plugins/ledmatrix-weather",
|
||||
"repo": "https://github.com/ChuckBuilds/ledmatrix-plugins",
|
||||
},
|
||||
{
|
||||
"id": "ledmatrix-flights",
|
||||
"name": "Flights",
|
||||
"plugin_path": "plugins/ledmatrix-flights",
|
||||
"repo": "https://github.com/ChuckBuilds/ledmatrix-plugins",
|
||||
},
|
||||
{
|
||||
"id": "third-party",
|
||||
"name": "Third Party",
|
||||
"plugin_path": "",
|
||||
"repo": "https://github.com/someone/thing",
|
||||
},
|
||||
]
|
||||
}
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def store(monkeypatch: pytest.MonkeyPatch) -> PluginStoreManager:
|
||||
manager = PluginStoreManager.__new__(PluginStoreManager)
|
||||
monkeypatch.setattr(manager, "fetch_registry", lambda *a, **k: REGISTRY, raising=False)
|
||||
return manager
|
||||
|
||||
|
||||
def _ids(entry: Optional[Dict[str, Any]]) -> Optional[str]:
|
||||
return entry.get("id") if entry else None
|
||||
|
||||
|
||||
class TestRegistryLookupByManifestId:
|
||||
def test_exact_registry_id_still_resolves(self, store: PluginStoreManager) -> None:
|
||||
assert _ids(store.get_registry_info("weather")) == "weather"
|
||||
|
||||
def test_manifest_id_resolves_via_plugin_path(self, store: PluginStoreManager) -> None:
|
||||
"""The case that broke restore: asked by the name on disk."""
|
||||
assert _ids(store.get_registry_info("ledmatrix-weather")) == "weather", (
|
||||
"a plugin installed as 'ledmatrix-weather' could not be found in a "
|
||||
"registry that lists it under plugin_path plugins/ledmatrix-weather")
|
||||
|
||||
def test_matching_id_and_path_unaffected(self, store: PluginStoreManager) -> None:
|
||||
assert _ids(store.get_registry_info("ledmatrix-flights")) == "ledmatrix-flights"
|
||||
|
||||
def test_unknown_plugin_still_returns_none(self, store: PluginStoreManager) -> None:
|
||||
assert store.get_registry_info("no-such-plugin") is None
|
||||
|
||||
def test_empty_plugin_path_is_not_a_wildcard(self, store: PluginStoreManager) -> None:
|
||||
"""Third-party entries carry plugin_path "" — that must not match ""."""
|
||||
assert store.get_registry_info("") is None
|
||||
|
||||
def test_exact_id_wins_over_a_path_match(self, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""If some other entry's path collides with a real id, id wins."""
|
||||
registry = {
|
||||
"plugins": [
|
||||
{"id": "decoy", "plugin_path": "plugins/weather"},
|
||||
{"id": "weather", "plugin_path": "plugins/ledmatrix-weather"},
|
||||
]
|
||||
}
|
||||
manager = PluginStoreManager.__new__(PluginStoreManager)
|
||||
monkeypatch.setattr(manager, "fetch_registry", lambda *a, **k: registry, raising=False)
|
||||
assert _ids(manager.get_registry_info("weather")) == "weather"
|
||||
@@ -7843,7 +7843,31 @@ def clear_old_errors():
|
||||
# Backup / Restore
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_BACKUP_EXPORT_DIR = PROJECT_ROOT / "config" / "backups" / "exports"
|
||||
def _resolve_backup_export_dir() -> Path:
|
||||
"""Where exported backups live: beside the install, not inside it.
|
||||
|
||||
They used to be written to ``<project>/config/backups/exports``. That is
|
||||
inside the directory a reinstall deletes, so the documented recovery path
|
||||
-- export a backup, then reinstall -- destroyed the backup it had just
|
||||
told the user to make. Anyone who downloaded the ZIP was fine; anyone
|
||||
relying on the on-device copy was not.
|
||||
|
||||
Falls back to the old location when the parent directory is not writable,
|
||||
so an unusual layout degrades to previous behaviour instead of failing to
|
||||
export at all.
|
||||
"""
|
||||
preferred = PROJECT_ROOT.parent / "ledmatrix-backups"
|
||||
try:
|
||||
preferred.mkdir(parents=True, exist_ok=True)
|
||||
probe = preferred / ".writetest"
|
||||
probe.write_text("", encoding="utf-8")
|
||||
probe.unlink()
|
||||
return preferred
|
||||
except OSError:
|
||||
return PROJECT_ROOT / "config" / "backups" / "exports"
|
||||
|
||||
|
||||
_BACKUP_EXPORT_DIR = _resolve_backup_export_dir()
|
||||
|
||||
|
||||
def _safe_backup_path(filename: str) -> Path:
|
||||
@@ -7997,7 +8021,22 @@ def backup_restore():
|
||||
|
||||
data = result.to_dict()
|
||||
if not result.success:
|
||||
return jsonify({'status': 'error', 'message': 'Restore had errors', 'data': data}), 500
|
||||
# Name what failed, and what nonetheless landed. A restore is
|
||||
# partial far more often than it is total -- a fresh install can
|
||||
# leave config_secrets.json unwritable by the web service, so
|
||||
# config restores and secrets do not. "Restore had errors" alone
|
||||
# left the user unable to tell a wholly failed restore from one
|
||||
# that quietly dropped their API keys.
|
||||
failed_plugins = [p.get('plugin_id') for p in (result.plugins_failed or [])]
|
||||
parts = []
|
||||
if result.restored:
|
||||
parts.append(f"restored: {', '.join(result.restored)}")
|
||||
if result.errors:
|
||||
parts.append(f"failed: {'; '.join(result.errors)}")
|
||||
if failed_plugins:
|
||||
parts.append(f"plugins not reinstalled: {', '.join(failed_plugins)}")
|
||||
message = 'Restore incomplete — ' + ('. '.join(parts) if parts else 'see logs')
|
||||
return jsonify({'status': 'error', 'message': message, 'data': data}), 500
|
||||
return jsonify({'status': 'success', 'data': data})
|
||||
except Exception as e:
|
||||
logger.error("backup_restore failed: %s", e, exc_info=True)
|
||||
|
||||
Reference in New Issue
Block a user