diff --git a/CHANGELOG.md b/CHANGELOG.md index 35b6bb0e..8b974026 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -488,6 +488,23 @@ policies are unchanged. ### Fixes +- A cache key too long to be a filename is now cached. The calendar + plugin's key joins every calendar id the user picked; on a real install + it passed 300 bytes, ext4 refuses names over 255, and every write failed + with `File name too long` — logged as "(permission denied)", so it read + like a cache-directory ownership problem. `DiskCache.get_cache_path` now + keeps a key of up to 200 UTF-8 bytes as its filename, as before, and + turns a longer one into its first bytes plus a hash of the whole key. The + web UI's cache list and delete keep working, because the shortened name + maps back to the same file. A failed write now names the real error. +- The cache's memory tier no longer serves data older than the reader asked + for. A record loaded from disk was timed in memory from the load, not + from when it was written, so `get(key, max_age=300)` could return data + close to 600 s old (after a restart, after the hourly memory sweep, or in + the other process, which only ever loads the record from disk), and a + stored `ttl` was stretched the same way. A memory hit is now also checked against + the record's own timestamp, and a stale one falls through to disk, which + returns a newer write if there is one. - An on-demand request that names a `*_live` mode now shows that mode. On ledpi, `{"plugin_id": "football-scoreboard", "mode": "ncaa_fb_live"}` with 15 college games on answered 200 and showed `nfl_recent`. The session's @@ -518,6 +535,16 @@ policies are unchanged. `DisplayManager.cleanup()` (reached from SIGTERM through `run()`'s `finally`) unregisters it with the frame recorder. New `frame_timing.uninstall_gc_monitor()`. +- The web interface's state subscription (`StateSubscription`, + `src/ipc/client.py`) resubscribes about 1 s after a display restart, every + time. Its reconnect wait went back to the minimum only when the + subscription was stopped. A disconnect after a working connection kept + doubling the wait, so successive display restarts were followed by waits + of 1, 2, 4, 8, 16 and then 30 s for good. + During each wait the web answered from one-shot `state.get` connections + instead of its copy. The wait now resets once a connection has stored a + snapshot. A display that does not offer the stream is still retried + slowly. - A plugin reload after a store update (`plugin.reload`, #720) no longer freezes the panel during Vegas. On ledpi a football reload froze it for 3.0 s (`Render stall over: no frame for 3043ms`). The reload ran on the @@ -534,6 +561,25 @@ policies are unchanged. for the plugin is refused (`plugin-reloading`), and a config reconcile neither loads it twice nor unloads it mid-load. A Vegas fetch that waited out a reload for the lock skips the old instance. +- A plugin display duration that is not a number no longer stops the + display. Several plugins (clock-simple, calendar, countdown) return their + `display_duration` setting as it is in config.json, so a value saved as + `"20"` or `null` (the raw config editor, a hand edit) reached the run loop + as a string or None. Comparing it with 0 raised a TypeError that no + handler in the loop caught: the display service exited when that plugin's + screen came up, and systemd restarted it into the same crash. The + controller now reads the plugin's answer as a number: a numeric string + counts, and anything else (or a `get_display_duration()` that raises) + shows the mode for 30 s, with one warning per plugin. +- A scroll strip narrower than the panel scrolls instead of raising on every + frame. When a frame ran off the end of the strip, `ScrollHelper` copied + the strip's tail and then the rest of the frame from its head, which + assumed the head was that wide; for a narrower strip that raised + `ValueError: could not broadcast` at every position, so nothing was drawn + and each frame logged a traceback. Vegas builds such a strip, with no + lead-in, when its content is narrower than the chain. A frame that runs + off the strip now continues from its head column by column, so a narrow + strip repeats across the panel; a wide strip wraps exactly as before. - The schedule-off blank and the WiFi notice no longer start with a scroller's leftovers. Both are drawn by the display controller rather than dispatched to a plugin, so #716's handover never reached them: drawn while @@ -543,6 +589,46 @@ policies are unchanged. scroller or Vegas) were counted as 0.5-1 s freezes and logged as a `Render stall ... mid-scroll`. The controller now ends the scroll state before drawing either. +- A plugin that keeps helpers in a package (elections' `providers/`, + flights' `enrichment/`, olympics' `data/` and `renderers/`) now runs its + updated helpers after a reload. Unloading dropped the package itself but + left its modules (`providers.feed`) in `sys.modules`, so the reload after a + store update imported the new `manager.py` and got the old helpers back from + the cache until the display restarted. `PluginLoader` now drops a plugin's + package modules when it unloads, and when a load fails part-way. +- Uninstalling a dev plugin that `scripts/dev/dev_plugin_setup.sh` linked + into the plugins directory now removes the link and leaves the checkout + alone. The store's removal passed the link to `shutil.rmtree`, which + refuses a symlink; its fallback then walked through the link and chmodded + every directory and file of the linked checkout to 0700, and the sudo stage + refused a path outside the plugins directory, so the uninstall failed with + the link still in place. The same removal discards the set-aside copy after + an install or update. A symlink, dangling or not, is now unlinked. +- A dev plugin linked in under a name its checkout does not share now loads. + `dev_plugin_setup.sh link-github foo ` clones `ledmatrix-foo` (the + repository naming convention) and links it as `plugins/foo`. The loader's + containment check for dependency installs resolved the link and looked for + `ledmatrix-foo` among the plugins directory's entries, found none, and + refused the plugin, so the load failed with "Dependency installation + failed" even when it had no `requirements.txt`. The check now looks for the + entry the path itself names in the plugins directory, the link, and still + only ever answers with an entry it found there. +- A plugin whose `update()` raises `asyncio.CancelledError` or `SystemExit` + no longer goes dark until a restart. Both derive from `BaseException`, not + `Exception`, and the update worker's bookkeeping caught only `Exception`: + the plugin kept its lock and stayed RUNNING, so it was never updated again + and every `display()` was skipped as busy. It is now recorded as that + update's failure, the same as any other raise. The plugin executor + reported such a call as a timeout; it now reports it as a failure. +- Saving a config change no longer freezes the panel while a plugin is busy. + `ConfigService` told its subscribers about a change while holding its lock, + and the display's per-plugin subscriber waits up to 5 s for a plugin in the + middle of an update. A save that enables or disables a plugin also queues a + reconcile, which the render thread runs, and its `get_config()` and + `unsubscribe()` waited behind every one of those callbacks. Subscribers now + run after the lock is released. One reload's notifications still finish + before the next one's start, and a callback `unsubscribe()` removed is not + running, and will not run, once it returns. - A plugin whose `display()` raises now opens its circuit breaker. The first frame of each screen goes through the plugin executor, which caught the exception and returned False. The display read that as "no content" and @@ -580,6 +666,48 @@ policies are unchanged. being stopped, blanks the panel within about a second. It used to stay on until the next minute, because the once-a-minute schedule check had already run that minute and the session had overridden its answer. +- Check & Update All updates what is installed now. A second run in the + same page sent the plugins the first run had seen, so a plugin uninstalled + since then failed with "plugin not found" and one installed since was + skipped. After a run the installed cards and the Updates badge show the + new versions; they kept offering "Update to vX" for what had just been + updated until the page was reloaded. +- The Run On-Demand dialog lists a plugin's display modes, so a mode other + than the first can be started, and pinned. `/api/v3/plugins/installed` + never sent `display_modes`, which the dialog reads, so every plugin + offered only its own id under "This plugin exposes a single display + mode", and the display started its first mode. Each entry now carries + `display_modes`, the modes its manifest declares. +- Installing Weather, Music, Stocks or Leaderboard from the Plugin Store + enables it, as installing any other plugin does. Each installs under the + id its manifest declares (`ledmatrix-weather` for the store's `weather`), + but the store enabled the store id, which `/api/v3/plugins/toggle` + answered with "Plugin not found": the plugin stayed disabled behind + "installed, but enabling it failed". `POST /api/v3/plugins/install` now + answers with the installed `plugin_id` (in the operation's result when it + is queued), and the store enables that. +- Reinstalling a plugin from the Plugin Store leaves it enabled or disabled + as it was. Reinstall enabled it as a fresh install does, so a plugin the + user had switched off came back on. +- A Plugin Store install that takes more than a minute is no longer + reported as failed. The store stopped waiting after 60 s and showed + "Install operation timed out" while the server, which allows the + plugin's dependency install 300 s on its own, carried on and usually + succeeded; the plugin was then neither enabled nor listed until the page + was reloaded. The store now waits up to 10 minutes, and if it still has + no answer it reloads the installed list and says the install may still + be running. +- The Plugin Store's category filter lists every category its plugins + have. It offered a fixed seven while the registry uses about twenty, so + plugins filed under productivity, utility, transit and the rest could not + be filtered to, and "Financial" missed the plugin filed under "finance". + The choices are now built from the store's plugins, as the Starlark + section's are. +- The Install button under Install Single Plugin (Plugin Manager > Install + from GitHub) runs one handler per click. It also had an inline `onclick` + whose handler threw a `ReferenceError` on every click; only the other + handler's request went out, and making the inline one work would have + sent every install twice. The inline handler is gone. - `/api/v3/plugins/installed` no longer reports the display's plugins as `live` while `/api/v3/health` says `display_loop: stalled`. The runtime snapshot is written from its own thread, which kept going while the render @@ -591,6 +719,17 @@ policies are unchanged. heartbeat when the service stops), is `stale` at once instead of `live` for up to 180 s. No new files or writes: both checks are on the reading side. +- A scoreboard's scroll and Vegas cards with `scroll_card.date_format: + "weekday"` now show the printed date's own weekday. A Friday 8 PM ET game + read "Sat Oct 2". The card took the weekday in the plugin's own + `timezone` setting, which ships blank, so it fell back to UTC, while the + "Oct 2" beside it came from the zone the plugin actually resolves (its + setting, then the global one, then the system zone). Every zone is within a + day of UTC, so the card now finds which day near the start's UTC date has + the printed month and day and names that one. Games east of UTC (Auckland, + Kiritimati) were off by a day the other way and are fixed the same way. + The switch-mode scorebug, which already used the plugin's resolved zone, + shares the same formatter and draws what it drew before. - `/api/v3/display/current-status` reflects a wake from scheduled-off, a schedule-off blank, or an on-demand session starting or ending at once, even when the mode name stays the same. The display republished its diff --git a/src/cache/disk_cache.py b/src/cache/disk_cache.py index a31782b9..e66f4b10 100644 --- a/src/cache/disk_cache.py +++ b/src/cache/disk_cache.py @@ -4,6 +4,7 @@ Disk Cache Handles persistent disk-based caching with atomic writes and error recovery. """ +import hashlib import json import math import os @@ -31,6 +32,35 @@ except ImportError: # pragma: no cover - exercised on hosts without the wheel # useful, and a half-written file was never useful. _ORPHAN_TEMP_MAX_AGE_SECONDS = 3600 +# Longest key, in UTF-8 bytes, used verbatim as a filename stem. ext4 caps a +# name at 255 bytes and set()'s temp file is "..json.<8 random>", 15 +# bytes longer than the stem, so anything near the cap could never be written: +# the calendar plugin's key joins every calendar id and passed 300 bytes on a +# real install, failing every write with ENAMETOOLONG. Longer keys keep this +# many bytes as a readable prefix and end in a hash of the whole key. +_MAX_KEY_FILENAME_BYTES = 200 +_KEY_HASH_CHARS = 16 + + +def _filename_stem(key: str) -> str: + """The filename stem for a key that is already a safe path component. + + Short keys are used as they are, so every file already on disk keeps its + name. A long one becomes its first bytes plus a hash of the full key: the + prefix keeps the stem recognisable (and keeps the data-type words that + cleanup's retention lookup reads from it), the hash keeps two keys that + share a long prefix apart. The result is itself short, so a stem read back + from a filename -- which is how the web UI names a key it deletes -- maps to + the same file. + """ + encoded = key.encode('utf-8') + if len(encoded) <= _MAX_KEY_FILENAME_BYTES: + return key + digest = hashlib.sha256(encoded).hexdigest()[:_KEY_HASH_CHARS] + keep = _MAX_KEY_FILENAME_BYTES - _KEY_HASH_CHARS - 1 + prefix = encoded[:keep].decode('utf-8', errors='ignore') + return f"{prefix}-{digest}" + class CacheStrategyProtocol(Protocol): @@ -343,6 +373,8 @@ class DiskCache: derives them), so rejecting anything with a path component turns away only inputs that could never have been written here. + A key too long to be a filename is shortened by _filename_stem. + Args: key: Cache key @@ -356,7 +388,7 @@ class DiskCache: if safe_key is None: self.logger.warning("Rejected unsafe cache key %r", key) return None - return os.path.join(self.cache_dir, f"{safe_key}.json") + return os.path.join(self.cache_dir, f"{_filename_stem(safe_key)}.json") def get(self, key: str, max_age: Optional[int] = 300) -> Optional[Dict[str, Any]]: """ @@ -561,7 +593,7 @@ class DiskCache: # If direct write also fails, try fallback location self.logger.warning("Direct write failed for key '%s' to %s: %s", key, cache_path, write_error) raise # Re-raise to trigger fallback logic - except (IOError, OSError, PermissionError): + except (IOError, OSError, PermissionError) as primary_error: # Attempt one-time fallback write to user's home cache directory try: # Try user's home cache directory as fallback @@ -587,11 +619,14 @@ class DiskCache: self.logger.debug("Fallback cache write also failed for key '%s': %s", key, e2) # If all write attempts failed, log warning but don't raise exception - # Cache is a performance optimization, not critical for operation + # Cache is a performance optimization, not critical for operation. + # Name the real error: this used to say "permission denied" + # whatever happened, which sent a too-long filename off to + # be debugged as a directory-ownership problem. self.logger.warning( - "Could not write cache for key '%s' to %s (permission denied). " + "Could not write cache for key '%s' to %s (%s). " "Cache will be unavailable for this key, but application will continue.", - key, cache_path + key, cache_path, primary_error.strerror or primary_error ) return # Exit gracefully without raising exception diff --git a/src/cache_manager.py b/src/cache_manager.py index 56fbdf9a..f18d2380 100644 --- a/src/cache_manager.py +++ b/src/cache_manager.py @@ -46,6 +46,32 @@ from src.cache.disk_cache import DateTimeEncoder # noqa: F401 - deliberate re-e # CacheManager.config_manager not built yet (None means "not available"). _UNSET: Any = object() + +def _outlived(record: Any, max_age: Optional[float], now: float) -> bool: + """Whether a record's own timestamp puts it past max_age. + + The memory tier times an entry from when it was put there, and a record + loaded from disk is put there when it is read, not when it was written: a + record 290 s old, read after a restart, could be served for another + max_age from memory. This is the age check DiskCache.get makes, with the + same rule that a stored ttl wins over the caller's max_age. A record that + carries no timestamp is left to the memory tier's own clock. + """ + if not isinstance(record, dict): + return False + stored_ttl = record.get('ttl') + if isinstance(stored_ttl, (int, float)) and not isinstance(stored_ttl, bool) \ + and stored_ttl >= 0: + max_age = stored_ttl + stamp = record.get('timestamp') + if max_age is None or stamp is None or isinstance(stamp, bool): + return False + try: + return now - float(stamp) > max_age + except (TypeError, ValueError): + return False + + class CacheManager: """Manages caching of API responses to reduce API calls.""" @@ -284,7 +310,11 @@ class CacheManager: # 1) Memory cache cached = self._memory_cache_component.get(key, max_age=in_memory_ttl) if cached is not None: - return cached + if not _outlived(cached, max_age, time.time()): + return cached + # Too old for this reader. Disk may hold a newer write (from the + # other process), and if it does not, the miss is the right answer. + self._memory_cache_component.clear(key) # 2) Disk cache record = self._disk_cache_component.get(key, max_age=max_age) @@ -318,7 +348,9 @@ class CacheManager: # Check memory cache first (1 minute TTL) cached = self._memory_cache_component.get(key, max_age=60) if cached is not None: - return cached + if not _outlived(cached, 3600, time.time()): + return cached + self._memory_cache_component.clear(key) # Check disk cache data = self._disk_cache_component.get(key, max_age=3600) # 1 hour for load_cache diff --git a/src/common/scroll_helper.py b/src/common/scroll_helper.py index e4da770d..b9caffeb 100644 --- a/src/common/scroll_helper.py +++ b/src/common/scroll_helper.py @@ -561,7 +561,7 @@ class ScrollHelper: width = self.display_width strip_width = self.cached_array.shape[1] - if start_x + width + 1 <= strip_width: + if 0 <= start_x and start_x + width + 1 <= strip_width: # Slice the backing array directly. Going via # _get_visible_portion_integer would build two PIL images only for # them to be converted straight back to arrays, which measured 15x @@ -569,9 +569,10 @@ class ScrollHelper: near = self.cached_array[:, start_x:start_x + width] far = self.cached_array[:, start_x + 1:start_x + 1 + width] else: - # Close enough to the end that one of the slices wraps; let the - # integer path handle that and pay the conversion. Continuous mode - # extends the strip before reaching here, so this is the rare case. + # One of the slices wraps (close to the end, or a strip narrower + # than the panel); let the integer path handle that and pay the + # conversion. Continuous mode extends the strip before reaching + # here, so this is the rare case. near = np.asarray( self._get_visible_portion_integer(start_x, start_x + width)) far = np.asarray( @@ -601,34 +602,33 @@ class ScrollHelper: _size = (self.display_width, self.display_height) img_w = self.cached_array.shape[1] - if end_x <= img_w: + if 0 <= start_x and end_x <= img_w: # Normal case: single contiguous slice (fastest path). tobytes() # on the column-slice view already returns C-order bytes, so # ascontiguousarray() first only added a second full-frame copy. return Image.frombytes( 'RGB', _size, self.cached_array[:, start_x:end_x].tobytes()) + + # Ensure frame buffer is allocated for all non-simple paths + if self._frame_buffer is None or self._frame_buffer.shape != (self.display_height, self.display_width, 3): + self._frame_buffer = np.zeros((self.display_height, self.display_width, 3), dtype=np.uint8) + + if img_w == 0: + self._frame_buffer[:] = 0 else: - # Ensure frame buffer is allocated for all non-simple paths - if self._frame_buffer is None or self._frame_buffer.shape != (self.display_height, self.display_width, 3): - self._frame_buffer = np.zeros((self.display_height, self.display_width, 3), dtype=np.uint8) + # The frame runs off the strip, so it carries on from the head: + # frame column j is strip column (start_x + j) modulo the strip's + # width -- the tail and then the head, and a strip narrower than + # the panel repeated across it. Copying the tail and then the rest + # of the frame from the head assumed the head was that wide, and + # raised at every position for a strip narrower than the panel + # (Vegas composes one, with no lead-in, when its content is + # narrower than the chain). + np.take(self.cached_array, np.arange(start_x, end_x), axis=1, + mode='wrap', out=self._frame_buffer) - width1 = img_w - start_x - if width1 > 0: - # Wrap-around: tail of image + head of image - self._frame_buffer[:, :width1] = self.cached_array[:, start_x:] - remaining_width = self.display_width - width1 - self._frame_buffer[:, width1:] = self.cached_array[:, :remaining_width] - else: - # Edge case: start_x at or past image end — show from beginning, - # clamped to available width (scroll_position should wrap before - # reaching this state in normal operation). - available = min(self.display_width, img_w) - self._frame_buffer[:, :available] = self.cached_array[:, :available] - if available < self.display_width: - self._frame_buffer[:, available:] = 0 - - return Image.frombytes('RGB', _size, self._frame_buffer.tobytes()) + return Image.frombytes('RGB', _size, self._frame_buffer.tobytes()) def calculate_dynamic_duration(self) -> int: """ diff --git a/src/common/sports_card.py b/src/common/sports_card.py index 0c3b7831..d33c4a36 100644 --- a/src/common/sports_card.py +++ b/src/common/sports_card.py @@ -18,7 +18,7 @@ the extra guard only stops a None size raising TypeError. """ import logging -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone from typing import Any, Dict, Optional, Tuple from zoneinfo import ZoneInfo @@ -338,10 +338,46 @@ def format_game_date(config: Optional[Dict[str, Any]], logger, date_text: str, if not raw: return "" fmt = str(scroll_card_option(config, "date_format", "abbrev") or "abbrev") - return _format_date_as(fmt, raw, lambda: weekday_for(config, logger, game)) + return _format_date_as(fmt, raw, lambda: weekday_for(config, logger, game), + game=game) -def _format_date_as(fmt: str, raw: str, weekday, months=MONTH_ABBR) -> str: +def _printed_weekday(game: Optional[Dict], month: int, day: int) -> str: + """The weekday of the date a card prints as month/day, or '' if unknown. + + The extractor prints "M/D" in the plugin's resolved zone (its own setting, + else the global one, else the system zone). The card cannot see that zone: + it is handed the plugin's config, whose ``timezone`` ships as "", so + card_tzinfo answers UTC and an evening kickoff in the Americas got the + next day's weekday ("Sat Oct 2" for a Friday game). Every zone is within + a day of UTC, so the printed date is the start's UTC date or a neighbour + of it; the one with that month and day is the date on the card. + """ + if not isinstance(game, dict): + return "" + raw = game.get("start_time_utc") or game.get("start_time") + if not raw: + return "" + try: + start = raw if isinstance(raw, datetime) else datetime.fromisoformat( + str(raw).replace("Z", "+00:00")) + if start.utcoffset() is None: + return "" # naive: no instant to place the date against + utc_day = start.astimezone(timezone.utc).date() + except (ValueError, TypeError, OverflowError): + return "" + for offset in (0, -1, 1): + try: + candidate = utc_day + timedelta(days=offset) + except OverflowError: + continue + if (candidate.month, candidate.day) == (month, day): + return WEEKDAY_ABBR[candidate.weekday()] + return "" + + +def _format_date_as(fmt: str, raw: str, weekday, months=MONTH_ABBR, + game: Optional[Dict] = None) -> str: """Render a stripped, non-empty "M/D" *raw* in style *fmt*. The body both date formatters share. They differ in which setting names the @@ -349,6 +385,9 @@ def _format_date_as(fmt: str, raw: str, weekday, months=MONTH_ABBR) -> str: ``SportsCoreSharedMixin._format_game_date``), so those arrive as arguments: *weekday* is a zero-argument callable, only called for the "weekday" style. *months* lets the mixin keep reading its (overridable) ``_MONTH_ABBR``. + With *game*, the "weekday" style names the printed date's own weekday + (:func:`_printed_weekday`), and *weekday* is only the fallback for a + date its start time cannot place. """ if fmt == "numeric": return raw @@ -364,7 +403,7 @@ def _format_date_as(fmt: str, raw: str, weekday, months=MONTH_ABBR) -> str: if fmt == "day_first": return f"{day} {name}" if fmt == "weekday": - day_name = weekday() + day_name = _printed_weekday(game, month, day) or weekday() return f"{day_name} {name} {day}" if day_name else f"{name} {day}" return f"{name} {day}" diff --git a/src/common/sports_shared.py b/src/common/sports_shared.py index c7a097f2..954a9b1f 100644 --- a/src/common/sports_shared.py +++ b/src/common/sports_shared.py @@ -360,14 +360,16 @@ class SportsCoreSharedMixin: The formatting is sports_card's. What differs from the card's ``format_game_date`` is passed in: the setting (``switch_date_format``, see :meth:`_switch_date_format`) and the weekday, which comes from - :meth:`_weekday_for` and so from this plugin's resolved timezone. + :meth:`_weekday_for` and so from this plugin's resolved timezone + when the game's start cannot place the printed date. The game goes + in too, so both formatters name the printed date's own weekday. """ raw = str(date_text or "").strip() if not raw: return raw return _card._format_date_as(self._switch_date_format(), raw, lambda: self._weekday_for(game), - self._MONTH_ABBR) + self._MONTH_ABBR, game=game) def _weekday_for(self, game: Optional[Dict]) -> str: """Weekday abbreviation from the game's start time, or ''.""" diff --git a/src/config_service.py b/src/config_service.py index 31822776..050ccc7b 100644 --- a/src/config_service.py +++ b/src/config_service.py @@ -14,7 +14,7 @@ import json import time import threading from pathlib import Path -from typing import Dict, Any, Optional, List, Callable +from typing import Dict, Any, Optional, List, Callable, Tuple from collections import defaultdict import logging import hashlib @@ -52,7 +52,18 @@ class ConfigService: # Thread safety self._lock: threading.RLock = threading.RLock() - + # Held across a whole reload -- read, swap, notify -- so one reload's + # notifications finish before the next one's start. Subscribers run + # under this lock and never under _lock: the display's per-plugin + # subscriber can wait seconds for a busy plugin, and get_config(), + # subscribe() and unsubscribe() -- called from the render thread -- + # must not wait behind it. + self._notify_lock: threading.RLock = threading.RLock() + # (key, callback, thread id) of the callback a notification is running, + # so unsubscribe() can wait for that one call; signalled on its return. + self._running_callback: Optional[Tuple[str, Callable[..., None], int]] = None + self._callback_done = threading.Condition(self._lock) + # Current configuration self._current_config: Dict[str, Any] = {} self._current_checksum: Optional[str] = None @@ -87,32 +98,33 @@ class ConfigService: True if config changed, False otherwise """ try: - new_config = self.config_manager.load_config() - new_checksum = self._calculate_checksum(new_config) - - with self._lock: - # Check if config actually changed - if new_checksum == self._current_checksum: - self.logger.debug("Configuration unchanged, skipping reload") - return False - - # Store old config for change detection - old_config = self._current_config.copy() - - # Update current config - self._current_config = new_config - self._current_checksum = new_checksum - - # Notify subscribers + with self._notify_lock: + new_config = self.config_manager.load_config() + new_checksum = self._calculate_checksum(new_config) + + with self._lock: + # Check if config actually changed + if new_checksum == self._current_checksum: + self.logger.debug("Configuration unchanged, skipping reload") + return False + + # Store old config for change detection + old_config = self._current_config.copy() + + # Update current config + self._current_config = new_config + self._current_checksum = new_checksum + + # Notify subscribers, outside _lock (see _notify_lock) self._notify_subscribers(old_config, new_config) - + self.logger.info( "Configuration reloaded (checksum: %s)", new_checksum[:8] ) - + return True - + except ConfigError as e: self.logger.error("Error loading configuration: %s", e, exc_info=True) return False @@ -127,35 +139,64 @@ class ConfigService: Args: old_config: Previous configuration new_config: New configuration + + Called without _lock held. The subscriber lists are copied under it, + and each callback is checked against them again just before it runs. """ + with self._lock: + subscribers = {key: list(callbacks) for key, callbacks in self._subscribers.items()} + # Notify global subscribers (key: '*') - for callback in self._subscribers.get('*', []): - try: - callback(old_config, new_config) - except Exception as e: - self.logger.error("Error in global config change callback: %s", e, exc_info=True) - + for callback in subscribers.get('*', []): + self._call_subscriber('*', callback, old_config, new_config) + # Notify plugin-specific subscribers - for plugin_id in self._subscribers.keys(): + for plugin_id, callbacks in subscribers.items(): if plugin_id == '*': continue - + old_plugin_config = old_config.get(plugin_id, {}) new_plugin_config = new_config.get(plugin_id, {}) - + # Only notify if plugin config actually changed if old_plugin_config != new_plugin_config: - for callback in self._subscribers[plugin_id]: - try: - callback(old_plugin_config, new_plugin_config) - except Exception as e: - self.logger.error( - "Error in config change callback for %s: %s", - plugin_id, - e, - exc_info=True - ) - + for callback in callbacks: + self._call_subscriber(plugin_id, callback, + old_plugin_config, new_plugin_config) + + def _call_subscriber( + self, + key: str, + callback: Callable[[Dict[str, Any], Dict[str, Any]], None], + old_config: Dict[str, Any], + new_config: Dict[str, Any], + ) -> None: + """Run one callback, unless it was unsubscribed since the snapshot. + + unsubscribe() promises that once it returns the callback is neither + running nor will run: the display unloads the plugin straight after. + """ + with self._lock: + if callback not in self._subscribers.get(key, ()): + return + self._running_callback = (key, callback, threading.get_ident()) + try: + callback(old_config, new_config) + except Exception as e: + if key == '*': + self.logger.error("Error in global config change callback: %s", e, exc_info=True) + else: + self.logger.error( + "Error in config change callback for %s: %s", + key, + e, + exc_info=True + ) + finally: + with self._lock: + self._running_callback = None + self._callback_done.notify_all() + def _check_file_changes(self) -> bool: """ Check if configuration files have been modified. @@ -276,6 +317,11 @@ class ConfigService: """ Unsubscribe from configuration changes. + Once this returns the callback is not running and will not be called + again. A notification that is running this very callback is waited + for (unless the callback is the caller); one running any other + callback is not. + Args: callback: Callback function to remove plugin_id: Optional plugin ID (must match subscription) @@ -285,6 +331,10 @@ class ConfigService: if callback in self._subscribers[key]: self._subscribers[key].remove(callback) self.logger.debug("Unsubscribed from config changes for %s", key) + while (self._running_callback is not None + and self._running_callback[:2] == (key, callback) + and self._running_callback[2] != threading.get_ident()): + self._callback_done.wait() def shutdown(self) -> None: """Shutdown the configuration service.""" diff --git a/src/display_controller.py b/src/display_controller.py index f4ac77d0..57bb03ea 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -25,11 +25,12 @@ import os import inspect import signal import json +import math import threading import types from collections import deque from contextlib import contextmanager -from typing import Dict, Any, List, Optional, Callable, Set, Tuple +from typing import Dict, Any, FrozenSet, List, Optional, Callable, Set, Tuple from datetime import datetime from concurrent.futures import ThreadPoolExecutor, as_completed # pylint: disable=no-name-in-module import pytz @@ -89,6 +90,19 @@ _MIN_INITIAL_UPDATE_TIMEOUT_SECONDS = 2.0 DEFAULT_DYNAMIC_DURATION_CAP = 180.0 +def _finite_seconds(value: Any) -> Optional[float]: + """``value`` as seconds when it is a finite number or a numeric string, + else None. A bool is not a number here, though it is an int: True would + read as a one-second screen.""" + if isinstance(value, bool): + return None + try: + seconds = float(value) + except (TypeError, ValueError, OverflowError): + return None + return seconds if math.isfinite(seconds) else None + + class _PluginReloadJob: """A ``plugin.reload`` whose slow half runs off the render thread. @@ -1348,6 +1362,12 @@ class DisplayController: "until one does", self.EMPTY_ROTATION_PAUSE) self._sleep_with_plugin_updates(self.EMPTY_ROTATION_PAUSE) + #: Plugins already warned about a display duration that is not a number, + #: so a bad setting logs once, not at every one of its screens. A + #: frozenset, replaced rather than mutated; class-level default for + #: controllers built without __init__ (tests). + _duration_warned: FrozenSet[str] = frozenset() + def _get_display_duration(self, mode_key): """Seconds to show a mode: the Rotation & Durations page's value for it (display.display_durations), else the plugin's own duration. @@ -1355,6 +1375,17 @@ class DisplayController: The saved value has to win. Every plugin inherits get_display_duration(), so checking the plugin first meant the page's values were never read. + + The plugin's answer is checked here, not trusted. Several plugins + return their display_duration setting straight from config.json, so + one saved as "20" or null (the raw config editor, a hand edit) came + back as a string or None; _resolve_durations compared it with 0, and + the TypeError went past every handler in the loop and stopped the + display service, which systemd restarted into the same screen. A + numeric string counts, as in BasePlugin.get_display_duration; any + other value that is not a finite number, or a raise, gets the 30 s a + mode without a plugin gets. A number at or below zero is passed on: + _resolve_durations has its own rule for that. """ display_durations = self.config.get('display', {}).get('display_durations', {}) or {} override = display_durations.get(mode_key) @@ -1362,8 +1393,22 @@ class DisplayController: return float(override) plugin_instance = self.plugin_modes.get(mode_key) - if plugin_instance is not None and hasattr(plugin_instance, 'get_display_duration'): - return plugin_instance.get_display_duration() + if plugin_instance is None or not hasattr(plugin_instance, 'get_display_duration'): + return 30 + try: + value = plugin_instance.get_display_duration() + except Exception as err: # pylint: disable=broad-except + problem = f"get_display_duration() raised {type(err).__name__}: {err}" + else: + seconds = _finite_seconds(value) + if seconds is not None: + return seconds + problem = f"display duration {value!r} is not a number" + plugin_id = getattr(plugin_instance, 'plugin_id', None) or mode_key + if plugin_id not in self._duration_warned: + self._duration_warned = self._duration_warned | {plugin_id} + logger.warning("Plugin %s: %s; showing its modes for 30s (logged once)", + plugin_id, problem) return 30 def _get_global_dynamic_cap(self) -> Optional[float]: diff --git a/src/ipc/client.py b/src/ipc/client.py index 169fdb41..82749727 100644 --- a/src/ipc/client.py +++ b/src/ipc/client.py @@ -396,9 +396,9 @@ class StateSubscription: def _run(self) -> None: backoff = _RECONNECT_MIN_SECONDS while not self._stop.is_set(): + snapshots = self.snapshots try: self._follow() - backoff = _RECONNECT_MIN_SECONDS except ControlError as e: self.last_error = e.reason if e.reason in _SLOW_RETRY_REASONS: @@ -414,6 +414,11 @@ class StateSubscription: sock.close() except OSError: pass + if self.snapshots != snapshots: + # This connection got as far as the display's state: whatever + # ended it (a restart, most often), it was working, so the + # next try starts from the shortest wait again. + backoff = _RECONNECT_MIN_SECONDS if self._stop.wait(backoff): return backoff = min(backoff * 2, _RECONNECT_MAX_SECONDS) diff --git a/src/plugin_system/plugin_executor.py b/src/plugin_system/plugin_executor.py index b8cfed15..2d1ca432 100644 --- a/src/plugin_system/plugin_executor.py +++ b/src/plugin_system/plugin_executor.py @@ -92,7 +92,10 @@ class PluginExecutor: with plugin_scope(plugin_id): result_container['value'] = operation() result_container['completed'] = True - except Exception as e: + except BaseException as e: # pylint: disable=broad-except + # asyncio.CancelledError and SystemExit too: uncaught, one + # ended this thread with 'completed' unset, and an operation + # that failed at once was reported as timing out. result_container['exception'] = e result_container['completed'] = True diff --git a/src/plugin_system/plugin_loader.py b/src/plugin_system/plugin_loader.py index c98b3256..0fea41b4 100644 --- a/src/plugin_system/plugin_loader.py +++ b/src/plugin_system/plugin_loader.py @@ -199,9 +199,22 @@ def contained_plugin_dir(plugin_dir: Path, plugins_dir: Path) -> Optional[str]: name that came out of ``os.scandir()`` on the trusted root carries no taint, which is a real containment guarantee (and one CodeQL's path-injection query can follow), not a string sanitiser. + + The entry looked for is the one ``plugin_dir`` itself names when it sits + directly in ``plugins_dir``: for a dev plugin symlinked in under its id, + the link's name. Resolving the link first and looking for the target's + folder name refused ``plugins/foo -> ~/.ledmatrix-dev-plugins/ledmatrix-foo`` + (what ``dev_plugin_setup.sh link-github foo `` makes), so the plugin + never loaded. Any other path is resolved and matched by its final name, + as before. """ - plugin_dir_real = os.path.realpath(str(plugin_dir)) plugins_dir_real = os.path.realpath(str(plugins_dir)) + plugin_dir_abs = os.path.abspath(str(plugin_dir)) + if os.path.realpath(os.path.dirname(plugin_dir_abs)) == plugins_dir_real: + matched_name = find_trusted_subdir(plugins_dir_real, os.path.basename(plugin_dir_abs)) + if matched_name is not None: + return os.path.join(plugins_dir_real, matched_name) + plugin_dir_real = os.path.realpath(str(plugin_dir)) matched_name = find_trusted_subdir(plugins_dir_real, os.path.basename(plugin_dir_real)) if matched_name is None: return None @@ -243,6 +256,10 @@ class PluginLoader: self.logger = logger or get_logger(__name__) self._loaded_modules: Dict[str, Any] = {} self._plugin_module_registry: Dict[str, set] = {} # Maps plugin_id to set of module names + # plugin_id -> {dotted name: module} for the modules of the plugin's + # own packages (``providers.feed``). They keep their names while the + # plugin runs and are dropped with it; see _iter_plugin_submodules. + self._plugin_submodules: Dict[str, Dict[str, Any]] = {} # Lock to serialize module loading when plugins share module names # (e.g., scroll_display.py, game_renderer.py across sport plugins). # During exec_module, bare-name sub-modules temporarily appear in @@ -449,6 +466,45 @@ class PluginLoader: continue return result + @staticmethod + def _iter_plugin_submodules( + plugin_dir: Path, before_keys: set + ) -> list: + """Return dotted-name modules from plugin_dir added after before_keys. + + The modules of a package the plugin ships (``providers.feed`` from + ``providers/feed.py``). _iter_plugin_bare_modules skips them, so the + bare ``providers`` was namespaced and dropped on unload while + ``providers.feed`` stayed in sys.modules: a reload after a store update + imported a fresh ``providers`` and then got the old ``feed`` back from + the cache, running the new manager.py against the old helpers until the + display restarted. + + A module counts when its ``__file__`` -- or, for a namespace package, + which has none, every ``__path__`` entry -- is inside plugin_dir, so a + library the plugin imports (``requests.adapters``) never does. + + Returns a list of (mod_name, module) tuples. + """ + resolved_dir = plugin_dir.resolve() + result = [] + for key in set(sys.modules.keys()) - before_keys: + if "." not in key: + continue + mod = sys.modules.get(key) + if mod is None: + continue + mod_file = getattr(mod, "__file__", None) + locations = [mod_file] if mod_file else list(getattr(mod, "__path__", None) or []) + if not locations: + continue + try: + if all(Path(loc).resolve().is_relative_to(resolved_dir) for loc in locations): + result.append((key, mod)) + except (ValueError, TypeError, OSError): + continue + return result + def _evict_stale_bare_modules(self, plugin_dir: Path) -> dict: """Temporarily remove bare-name sys.modules entries from other plugins. @@ -527,6 +583,13 @@ class PluginLoader: # Track for cleanup during unload self._plugin_module_registry[plugin_id] = namespaced_names + # The modules of the plugin's own packages keep their dotted names + # while it runs -- as they always have, so the package and its + # children stay a matching set in sys.modules -- and are dropped + # with the plugin by unregister_plugin_modules(). + self._plugin_submodules[plugin_id] = dict( + self._iter_plugin_submodules(plugin_dir, before_keys)) + if namespaced_names: self.logger.info( "Namespace-isolated %d module(s) for plugin %s", @@ -537,10 +600,16 @@ class PluginLoader: """Remove namespaced sub-modules and cached module for a plugin from sys.modules. Called by PluginManager during unload to clean up all module entries - that were created when the plugin was loaded. + that were created when the plugin was loaded, including the dotted + modules of its packages. A dotted name is dropped only while it still + holds this plugin's module: the name is not namespaced, so another + plugin may have put its own there since. """ for ns_name in self._plugin_module_registry.pop(plugin_id, set()): sys.modules.pop(ns_name, None) + for name, mod in self._plugin_submodules.pop(plugin_id, {}).items(): + if sys.modules.get(name) is mod: + sys.modules.pop(name, None) self._loaded_modules.pop(plugin_id, None) def load_module( @@ -646,11 +715,13 @@ class PluginLoader: if evicted_name not in sys.modules: sys.modules[evicted_name] = evicted_mod # Clean up the partially-initialized main module and any - # bare-name sub-modules that were added during exec_module - # so they don't leak into subsequent plugin loads. + # bare-name or package sub-modules that were added during + # exec_module so they don't leak into subsequent plugin loads. sys.modules.pop(module_name, None) for key, _ in self._iter_plugin_bare_modules(plugin_dir, before_keys): sys.modules.pop(key, None) + for key, _ in self._iter_plugin_submodules(plugin_dir, before_keys): + sys.modules.pop(key, None) raise self._loaded_modules[plugin_id] = module diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 79d689b3..69c9548b 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -1163,7 +1163,7 @@ class PluginManager: def _record_update_failure( self, plugin_id: str, - exc: Optional[Exception] = None, + exc: Optional[BaseException] = None, log: bool = True, count_failure: bool = True, ) -> None: @@ -1187,7 +1187,7 @@ class PluginManager: """ failure_time = time.time() if exc is not None: - err: Exception = exc + err: BaseException = exc error_type = type(exc).__name__ else: err = Exception(f"Plugin {plugin_id} execution failed (timeout or executor error)") @@ -1653,7 +1653,7 @@ class PluginManager: finish_guard = threading.Lock() finished = {'done': False} - def _finish(success: bool, exc: Optional[Exception] = None) -> None: + def _finish(success: bool, exc: Optional[BaseException] = None) -> None: with finish_guard: if finished['done']: return @@ -1727,7 +1727,13 @@ class PluginManager: self.resource_monitor.monitor_call(plugin_id, plugin_instance.update) else: plugin_instance.update() - except Exception as exc: + except BaseException as exc: # pylint: disable=broad-except + # BaseException, not just Exception: asyncio.CancelledError + # and SystemExit derive from it. Either one skipped _finish, + # so the plugin kept its lock and stayed RUNNING for good -- + # never rescheduled, and every display() skipped as busy. + # Re-raised for the executor, which reports it as this + # update's failure. _finish(False, exc=exc) raise else: diff --git a/src/plugin_system/store_manager.py b/src/plugin_system/store_manager.py index d7b32e68..80f60f67 100644 --- a/src/plugin_system/store_manager.py +++ b/src/plugin_system/store_manager.py @@ -344,12 +344,26 @@ class PluginStoreManager(_RegistryMixin, _InstallMixin, _UpdateMixin): 2. Fix permissions via os.chmod() then retry (works for same-owner files) 3. Use sudo rm -rf as last resort (works for root-owned __pycache__, etc.) + A symlink -- a dev plugin linked in by scripts/dev/dev_plugin_setup.sh + -- is removed as a link, before any of that: rmtree refuses one, and + stage 2 would walk through it and chmod the developer's checkout. + Args: path: Path to directory to remove Returns: True if directory was removed successfully, False otherwise """ + if path.is_symlink(): + # Checked before exists(), which follows the link: a dangling one + # would read as already removed and be left behind. + try: + path.unlink() + return True + except OSError as e: + self.logger.error(f"Could not remove the symlink {path}: {e}") + return False + if not path.exists(): return True # Already removed diff --git a/test/js/README.md b/test/js/README.md index cc0c1b4f..c28d2de8 100644 --- a/test/js/README.md +++ b/test/js/README.md @@ -45,6 +45,10 @@ server has none. |---|---|---| | `unit/test_list_filter.js` | no | `ListFilter` search/filter/sort/count/sticky, and the installed-plugins config **extracted verbatim** from `plugins_manager.js` so the test can't drift from it | | `unit/test_update_all.js` | no | `PluginInstallManager.updateAll` from `plugins/install_manager.js`: Check & Update All sends only plugin ids (never `starlark:` app entries), re-sends a request that got no HTTP answer (web service restarting) instead of skipping that plugin, never re-sends one that got any HTTP answer (the real `api_client.js` classifies a proxy 502 or a JSON error without `error_code` as `API_ERROR`), and counts a no-op update as already up to date in the summary. Also run by `test/web_interface/test_update_all_plugins.py` so CI covers it | +| `unit/test_store_install.js` | no | The store's Install button, with the whole of `plugins_manager.js` run by `plugins_manager_sandbox.js` (a vm context, fake DOM and API): a fresh install reloads the list, then enables the id the plugin was installed as -- the answer's `plugin_id`, else the installed entry the store entry matches (Weather installs as `ledmatrix-weather`); a Reinstall leaves the enabled state alone | +| `unit/test_install_polling.js` | no | How long Install waits for a queued install (sandbox): at least the server's 300 s dependency-install timeout; when it stops waiting it reloads the installed list and warns, rather than reporting a failure or enabling anything | +| `unit/test_store_categories.js` | no | The store's category filter (sandbox): the template ships only All Categories, the rest come from the store's plugins (one per category whatever its case), choosing one filters to it, and a swapped-in select is refilled from the cache keeping the choice | +| `unit/test_github_url_install.js` | no | Install Single Plugin (sandbox, the button as `plugins.html` ships it): no inline `onclick`, so a click or Enter sends exactly one `install-from-url` request and raises no error | | `unit/test_render_cards.js` | no | `renderInstalledCards` markup, both empty states, and HTML-escaping of hostile plugin metadata | | `unit/test_style_editor_element_keys.js` | no | `elementKeys()`/`styleRows()`/`positionRows()` from `widgets/style-editor.js`: every `customization.layout` entry gets exactly one row -- paired with its style element through core's `x-layout-key` (so `score` belongs to `score_text`, not a second row), or a position row of its own, leaves included -- since the widget claims the whole `layout` block from the generic fallback renderer | | `unit/test_style_editor_layout_leaf_columns.js` | no | `columnsFor()` from `widgets/style-editor.js`: a layout-only key whose own value is a leaf (no x/y sub-object, e.g. a `show_logo` toggle) gets a self-keyed column instead of a blank, uneditable row | diff --git a/test/js/plugins_manager_sandbox.js b/test/js/plugins_manager_sandbox.js new file mode 100644 index 00000000..4bd48579 --- /dev/null +++ b/test/js/plugins_manager_sandbox.js @@ -0,0 +1,212 @@ +// The whole of plugins_manager.js (and list_filter.js before it, as the page +// loads them), evaluated in a node vm context against a small fake DOM. +// +// For suites that drive the plugin manager's real flows -- install, polling, +// store filters, the GitHub-URL button -- rather than one function sliced +// out of the file. Nothing is mocked inside the script: only what the page +// gives it (document, fetch, timers, showNotification, LEDEscape). +// +// const sb = create({ route: (method, url, body) => ({ status, json }) }); +// sb.el('plugin-store-grid'); // make an element exist by id +// sb.window.installPlugin('weather'); +// await sb.until(() => sb.requests.some(r => r.url.includes('/toggle'))); +// +// Timers ignore their delays and run on the next turn, so a poll loop that +// would take minutes in a browser finishes in milliseconds. The page is in +// readyState "loading" with no #installed-plugins-grid, so the script's own +// start-up does nothing until a suite asks for it (window.initPluginsPage()). +const fs = require('fs'); +const path = require('path'); +const vm = require('vm'); +const ledEscape = require('./led_escape'); + +const V3 = path.resolve(__dirname, '../../web_interface/static/v3'); +const PLUGINS_HTML = path.resolve(__dirname, '../../web_interface/templates/v3/partials/plugins.html'); + +class FakeClassList { + constructor() { this.set = new Set(); } + add(...c) { c.forEach(x => this.set.add(x)); } + remove(...c) { c.forEach(x => this.set.delete(x)); } + contains(c) { return this.set.has(c); } + toggle(c, force) { + const on = force === undefined ? !this.set.has(c) : !!force; + if (on) this.set.add(c); else this.set.delete(c); + return on; + } +} + +function create({ route } = {}) { + const elements = new Map(); + const requests = []; + const toasts = []; + const errors = []; + const restartNotes = []; + + class FakeElement { + constructor(id, tag = 'div', attributes = {}) { + this.id = id; + this.tagName = tag.toUpperCase(); + this.attributes = { ...attributes }; + this.listeners = {}; + this.children = []; + this.classList = new FakeClassList(); + this.style = { removeProperty() {} }; + this.dataset = {}; + this.value = ''; + this.textContent = ''; + this.disabled = false; + this.parentNode = null; + this._html = ''; + } + get innerHTML() { return this._html; } + set innerHTML(v) { this._html = String(v); this.children = []; } + getAttribute(n) { return n in this.attributes ? this.attributes[n] : null; } + setAttribute(n, v) { this.attributes[n] = String(v); } + hasAttribute(n) { return n in this.attributes; } + removeAttribute(n) { delete this.attributes[n]; } + addEventListener(type, fn) { (this.listeners[type] = this.listeners[type] || []).push(fn); } + removeEventListener(type, fn) { + this.listeners[type] = (this.listeners[type] || []).filter(f => f !== fn); + } + appendChild(child) { this.children.push(child); child.parentNode = this; return child; } + querySelector() { return null; } + querySelectorAll() { return []; } + closest() { return null; } + cloneNode() { + const copy = new FakeElement(this.id, this.tagName, this.attributes); + copy._html = this._html; + copy.value = this.value; + return copy; + } + replaceChild(next, prev) { + next.parentNode = this; + prev.parentNode = null; + if (next.id) elements.set(next.id, next); + return prev; + } + replaceWith(next) { if (this.parentNode) this.parentNode.replaceChild(next, this); } + // A browser runs an inline on attribute first (it was set before + // any listener was added), then the listeners, and an exception in one + // does not stop the next: it is reported, which is what `errors` holds. + dispatch(type, init = {}) { + const event = { + type, target: this, currentTarget: this, key: init.key, + defaultPrevented: false, + preventDefault() { this.defaultPrevented = true; }, + stopPropagation() {}, stopImmediatePropagation() {}, + }; + const inline = this.getAttribute('on' + type); + const handlers = []; + if (inline !== null) { + handlers.push(vm.runInContext(`(function(event) {\n${inline}\n})`, ctx)); + } + handlers.push(...(this.listeners[type] || [])); + for (const h of handlers) { + try { h.call(this, event); } catch (e) { errors.push(e); } + } + return event; + } + click() { return this.dispatch('click'); } + } + + function el(id, tag, attributes) { + if (!elements.has(id)) { + const parent = new FakeElement(null); + parent.appendChild(new FakeElement(id, tag, attributes)); + elements.set(id, parent.children[0]); + } + return elements.get(id); + } + + const timers = []; + const ctx = { + // Warnings are the script noting elements this fake page doesn't have. + console: { log: console.log.bind(console), error: console.error.bind(console), + warn: () => {}, info: () => {}, debug: () => {} }, + debugLog: () => {}, + addEventListener() {}, + URL, + document: { + readyState: 'loading', + body: { addEventListener() {} }, + getElementById: id => elements.get(id) || null, + querySelector: () => null, + querySelectorAll: () => [], + addEventListener() {}, + dispatchEvent() { return true; }, + createElement: tag => new FakeElement(null, tag), + }, + CustomEvent: class { constructor(type, init) { this.type = type; this.detail = init && init.detail; } }, + setTimeout: (fn, _ms, ...args) => { timers.push(setImmediate(() => fn(...args))); return timers.length; }, + clearTimeout: () => {}, + setInterval: () => 0, + clearInterval: () => {}, + requestAnimationFrame: fn => setImmediate(fn), + getComputedStyle: () => ({ display: 'block' }), + scrollTo() {}, + sessionStorage: { getItem: () => null, setItem() {}, removeItem() {} }, + localStorage: { getItem: () => null, setItem() {}, removeItem() {} }, + confirm: () => true, + alert: () => {}, + showNotification: (message, type) => { + toasts.push({ message: String(message), + type: type && typeof type === 'object' ? type.type : type }); + }, + noteRestartRequired: (body) => { restartNotes.push(body); }, + fetch: async (url, opts = {}) => { + const method = (opts.method || 'GET').toUpperCase(); + let body = null; + try { body = opts.body ? JSON.parse(opts.body) : null; } catch (e) { body = opts.body; } + requests.push({ method, url: String(url), body }); + const answer = (route && route(method, String(url), body)) || { status: 200, json: { status: 'success' } }; + const status = answer.status || 200; + return { ok: status < 400, status, json: async () => answer.json }; + }, + }; + ctx.window = ctx; + vm.createContext(ctx); + ledEscape.install(ctx); + for (const file of ['js/plugins/list_filter.js', 'plugins_manager.js']) { + vm.runInContext(fs.readFileSync(path.join(V3, file), 'utf8'), ctx, { filename: file }); + } + + // Resolves once cond() is true, letting timers and promises run between + // checks; rejects if it never is. + async function until(cond, label = 'condition', turns = 20000) { + for (let i = 0; i < turns; i++) { + if (cond()) return; + await new Promise(r => setImmediate(r)); + } + throw new Error('timed out waiting for ' + label); + } + + // Lets every pending timer and promise run. + async function settle(turns = 50) { + for (let i = 0; i < turns; i++) await new Promise(r => setImmediate(r)); + } + + return { window: ctx, el, FakeElement, requests, toasts, errors, restartNotes, until, settle }; +} + +// The attributes of the element with this id in partials/plugins.html, as +// the template ships them (no Jinja on the tags these suites read). +function templateAttributes(id) { + const html = fs.readFileSync(PLUGINS_HTML, 'utf8'); + const at = html.indexOf(`id="${id}"`); + if (at < 0) throw new Error(`no element with id ${id} in plugins.html`); + const start = html.lastIndexOf('<', at); + let end = start, quote = null; + for (; end < html.length; end++) { + const ch = html[end]; + if (quote) { if (ch === quote) quote = null; } else if (ch === '"' || ch === "'") quote = ch; + else if (ch === '>') break; + } + const tag = html.slice(start, end + 1); + const attrs = {}; + const re = /([\w:-]+)\s*=\s*("([^"]*)"|'([^']*)')/g; + let m; + while ((m = re.exec(tag))) attrs[m[1]] = m[3] !== undefined ? m[3] : m[4]; + return { tag: tag.match(/^<(\w+)/)[1], attrs, source: tag }; +} + +module.exports = { create, templateAttributes }; diff --git a/test/js/run_all.js b/test/js/run_all.js index 41122501..1e6a5437 100755 --- a/test/js/run_all.js +++ b/test/js/run_all.js @@ -20,7 +20,12 @@ const UNIT = ['unit/test_list_filter.js', 'unit/test_render_cards.js', 'unit/test_html_escaping.js', 'unit/test_style_editor_element_keys.js', 'unit/test_style_editor_layout_leaf_columns.js', 'unit/test_style_editor_layout_leaf_collision.js', - 'unit/test_update_all.js', 'unit/test_inline_handler_escaping.js', + 'unit/test_update_all.js', + 'unit/test_store_install.js', + 'unit/test_install_polling.js', + 'unit/test_store_categories.js', + 'unit/test_github_url_install.js', + 'unit/test_inline_handler_escaping.js', 'unit/test_plugin_action_delegation.js', 'unit/test_file_upload_widget.js', 'unit/test_store_registry_fields.js', 'unit/test_restart_banner.js', 'unit/test_page_registry.js', 'unit/test_core_modules.js']; diff --git a/test/js/unit/test_github_url_install.js b/test/js/unit/test_github_url_install.js new file mode 100644 index 00000000..048a6677 --- /dev/null +++ b/test/js/unit/test_github_url_install.js @@ -0,0 +1,93 @@ +// Plugin Manager > Install from GitHub > Install Single Plugin: one click, +// one request, no errors. +// +// The Install button carried an inline onclick calling +// window.handleGitHubPluginInstall, and attachInstallButtonHandler also gave +// it a click listener that installs. Both ran on every click. The inline one +// threw a ReferenceError (it called isGithubUrl, which lives inside the +// plugin-manager IIFE, from outside it), so only the listener's request went +// out -- and fixing that scope alone would have sent every install twice. +// The button now has the listener only. +// +// Runs the whole of plugins_manager.js in the sandbox, with the button as +// partials/plugins.html ships it. + +const { create, templateAttributes } = require('../plugins_manager_sandbox'); + +let pass = 0, fail = 0; +const ok = (label, cond, extra) => cond + ? (pass++, console.log(' ok ' + label)) + : (fail++, console.log(' FAIL ' + label + (extra !== undefined ? ' -> ' + JSON.stringify(extra).slice(0, 400) : ''))); + +const URL = 'https://github.com/someone/ledmatrix-demo'; + +function route(method, url) { + if (method === 'POST' && url === '/api/v3/plugins/install-from-url') { + return { json: { status: 'success', message: 'Plugin demo installed successfully', plugin_id: 'demo' } }; + } + if (url.startsWith('/api/v3/plugins/installed')) return { json: { status: 'success', data: { plugins: [] } } }; + return { json: { status: 'success' } }; +} + +function page() { + const sb = create({ route }); + const button = templateAttributes('install-plugin-from-url'); + sb.el('install-plugin-from-url', button.tag, button.attrs); + sb.el('github-plugin-url', 'input'); + sb.el('github-plugin-status'); + sb.el('plugin-branch-input', 'input'); + return sb; +} + +const installs = sb => sb.requests.filter(r => r.url === '/api/v3/plugins/install-from-url'); + +(async () => { + console.log('\nthe template'); + { + const { attrs } = templateAttributes('install-plugin-from-url'); + ok('the Install button has no inline onclick', !('onclick' in attrs), attrs.onclick); + } + + console.log('\na click'); + { + const sb = page(); + sb.window.attachInstallButtonHandler(); + // htmx:afterSettle runs it again on every swap; that must not add a handler. + sb.window.attachInstallButtonHandler(); + sb.el('github-plugin-url').value = URL; + sb.window.document.getElementById('install-plugin-from-url').click(); + await sb.settle(); + ok('raises no error', sb.errors.length === 0, sb.errors.map(String)); + ok('sends exactly one install request', installs(sb).length === 1, installs(sb)); + ok('for the URL typed', installs(sb)[0] && installs(sb)[0].body.repo_url === URL, installs(sb)); + ok('and reports the result', /Successfully installed: demo/.test(sb.el('github-plugin-status').innerHTML), + sb.el('github-plugin-status').innerHTML); + } + + console.log('\nEnter in the URL field'); + { + const sb = page(); + sb.window.attachInstallButtonHandler(); + const input = sb.el('github-plugin-url'); + input.value = URL; + input.dispatch('keypress', { key: 'Enter' }); + await sb.settle(); + ok('raises no error', sb.errors.length === 0, sb.errors.map(String)); + ok('sends exactly one install request', installs(sb).length === 1, installs(sb)); + } + + console.log('\na URL that is not GitHub'); + { + const sb = page(); + sb.window.attachInstallButtonHandler(); + sb.el('github-plugin-url').value = 'https://example.com/x'; + sb.window.document.getElementById('install-plugin-from-url').click(); + await sb.settle(); + ok('is refused without a request or an error', + installs(sb).length === 0 && sb.errors.length === 0 && /valid GitHub URL/.test(sb.el('github-plugin-status').innerHTML), + { errors: sb.errors.map(String), status: sb.el('github-plugin-status').innerHTML }); + } + + console.log(`\n${pass} passed, ${fail} failed`); + process.exit(fail ? 1 : 0); +})().catch(e => { console.error(e); process.exit(1); }); diff --git a/test/js/unit/test_install_polling.js b/test/js/unit/test_install_polling.js new file mode 100644 index 00000000..253edbc4 --- /dev/null +++ b/test/js/unit/test_install_polling.js @@ -0,0 +1,89 @@ +// How long the store's Install waits for a queued install, and what it says +// when it stops waiting. +// +// It polled the operation 60 times, a second apart, then reported "Install +// operation timed out" as an error and did nothing else. The server is +// allowed far longer: the plugin's dependency install alone may take 300 s +// (install_requirements_file in src/plugin_system/store_install.py), after +// a download that fetches the plugin one file at a time. So an install that +// went on to succeed was reported as failed, never enabled, and missing from +// the installed list until the page was reloaded. +// +// Runs the whole of plugins_manager.js in the sandbox; its timers ignore +// their delays, so each poll here stands for one second on a real page. + +const { create } = require('../plugins_manager_sandbox'); + +let pass = 0, fail = 0; +const ok = (label, cond, extra) => cond + ? (pass++, console.log(' ok ' + label)) + : (fail++, console.log(' FAIL ' + label + (extra !== undefined ? ' -> ' + JSON.stringify(extra).slice(0, 400) : ''))); + +// The server's dependency-install timeout, in polls (one a second). +const DEPENDENCY_INSTALL_TIMEOUT_POLLS = 300; + +function server(completesAfterPolls) { + const state = { polls: 0, installed: [] }; + state.route = (method, url, body) => { + if (url.startsWith('/api/v3/plugins/installed')) { + return { json: { status: 'success', data: { plugins: state.installed.map(p => ({ ...p })) } } }; + } + if (method === 'POST' && url === '/api/v3/plugins/install') { + return { json: { status: 'success', message: 'queued', data: { operation_id: 'op-1' } } }; + } + if (url === '/api/v3/plugins/operation/op-1') { + state.polls++; + if (completesAfterPolls === null || state.polls < completesAfterPolls) { + return { json: { status: 'success', data: { status: 'running' } } }; + } + state.installed = [{ id: 'clock-simple', name: 'Clock', enabled: false }]; + return { json: { status: 'success', data: { status: 'completed', + result: { success: true, message: 'installed', plugin_id: 'clock-simple' } } } }; + } + if (method === 'POST' && url === '/api/v3/plugins/toggle') { + return { json: { status: 'success', message: 'enabled' } }; + } + return { json: { status: 'success' } }; + }; + return state; +} + +(async () => { + console.log('\nan install that takes longer than a minute'); + { + // 200 s: well inside what the server allows. + const srv = server(200); + const sb = create({ route: srv.route }); + sb.window.installPlugin('clock-simple'); + await sb.until(() => sb.toasts.some(t => /installed and enabled|enabling it failed|timed out|still/i.test(t.message)), + 'the install to finish'); + await sb.settle(); + ok('is waited for until it completes', srv.polls === 200, srv.polls); + ok('is not reported as an error', !sb.toasts.some(t => t.type === 'error'), sb.toasts); + ok('and is enabled', sb.requests.some(r => r.url === '/api/v3/plugins/toggle' && r.body.plugin_id === 'clock-simple'), + sb.requests.filter(r => r.method === 'POST')); + } + + console.log('\nan install that never reports back'); + { + const srv = server(null); + const sb = create({ route: srv.route }); + sb.window.installPlugin('clock-simple'); + await sb.until(() => sb.toasts.length >= 3, 'the poller to give up'); + await sb.settle(); + ok(`is polled for at least the ${DEPENDENCY_INSTALL_TIMEOUT_POLLS} s dependency-install timeout`, + srv.polls >= DEPENDENCY_INSTALL_TIMEOUT_POLLS, srv.polls); + ok('...but not forever', srv.polls <= 1200, srv.polls); + const lastPoll = sb.requests.map(r => r.url).lastIndexOf('/api/v3/plugins/operation/op-1'); + ok('then the installed list is reloaded, to show what actually happened', + sb.requests.slice(lastPoll + 1).some(r => r.url === '/api/v3/plugins/installed'), + sb.requests.slice(lastPoll + 1).map(r => r.url)); + const last = sb.toasts[sb.toasts.length - 1]; + ok('it says the install may still be running, as a warning, not a failure', + last && last.type === 'warning' && !/fail|timed out/i.test(last.message), sb.toasts); + ok('nothing is enabled on a guess', !sb.requests.some(r => r.url === '/api/v3/plugins/toggle')); + } + + console.log(`\n${pass} passed, ${fail} failed`); + process.exit(fail ? 1 : 0); +})().catch(e => { console.error(e); process.exit(1); }); diff --git a/test/js/unit/test_store_categories.js b/test/js/unit/test_store_categories.js new file mode 100644 index 00000000..f5195012 --- /dev/null +++ b/test/js/unit/test_store_categories.js @@ -0,0 +1,105 @@ +// The Plugin Store's category filter offers the categories its plugins have. +// +// The template listed seven fixed categories. The registry uses about +// twenty (productivity, utility, transit, finance, ...), so roughly a third +// of the store could not be filtered to at all, and "Financial" missed the +// plugin filed under "finance". The options are now built from the store's +// plugins, as the Starlark section builds its own; the template ships only +// "All Categories". +// +// Runs the whole of plugins_manager.js in the sandbox. + +const fs = require('fs'); +const path = require('path'); +const { create, templateAttributes } = require('../plugins_manager_sandbox'); + +let pass = 0, fail = 0; +const ok = (label, cond, extra) => cond + ? (pass++, console.log(' ok ' + label)) + : (fail++, console.log(' FAIL ' + label + (extra !== undefined ? ' -> ' + JSON.stringify(extra).slice(0, 400) : ''))); + +const STORE = [ + { id: 'nfl', name: 'NFL', category: 'sports' }, + { id: 'nba', name: 'NBA', category: 'Sports' }, + { id: 'todo', name: 'Todo', category: 'productivity' }, + { id: 'stocks', name: 'Stocks', category: 'finance' }, + { id: 'crypto', name: 'Crypto', category: 'financial' }, + { id: 'bus', name: 'Bus', category: 'transit' }, + { id: 'mystery', name: 'Mystery' }, +]; + +function route(method, url) { + if (url.startsWith('/api/v3/plugins/store/list')) return { json: { status: 'success', data: { plugins: STORE } } }; + if (url.startsWith('/api/v3/plugins/installed')) return { json: { status: 'success', data: { plugins: [] } } }; + if (url.startsWith('/api/v3/plugins/store/github-status')) { + return { json: { status: 'success', data: { token_status: 'valid', authenticated: true, rate_limit: 5000 } } }; + } + if (url.startsWith('/api/v3/plugins/saved-repositories')) { + return { json: { status: 'success', data: { repositories: [] } } }; + } + if (url.startsWith('/api/v3/display/on-demand/status')) { + return { json: { status: 'success', data: { state: {}, service: {} } } }; + } + return { json: { status: 'success' } }; +} + +const options = sel => sel.children.map(o => o.value); +const cardIds = sb => [...sb.el('plugin-store-grid').innerHTML.matchAll(/]*>([^<]*)<\/h4>/g)].map(m => m[1]); + +(async () => { + console.log('\nthe template'); + { + const html = fs.readFileSync(path.resolve(__dirname, + '../../../web_interface/templates/v3/partials/plugins.html'), 'utf8'); + const start = html.indexOf(' from the template, the store list still cached. + const fresh = new sb.FakeElement('plugin-category', 'select', attrs); + select.parentNode.replaceChild(fresh, select); + sb.window.searchPluginStore(false); + await sb.settle(); + ok('the new select is filled from the cache', + JSON.stringify(options(fresh)) === JSON.stringify(['finance', 'financial', 'productivity', 'sports', 'transit']), + options(fresh)); + ok('keeping the chosen category', fresh.value === 'sports', fresh.value); + } + + console.log(`\n${pass} passed, ${fail} failed`); + process.exit(fail ? 1 : 0); +})().catch(e => { console.error(e); process.exit(1); }); diff --git a/test/js/unit/test_store_install.js b/test/js/unit/test_store_install.js new file mode 100644 index 00000000..ac421e8f --- /dev/null +++ b/test/js/unit/test_store_install.js @@ -0,0 +1,138 @@ +// The store's Install button: which plugin it enables afterwards, and when. +// +// 1. Weather, Music, Stocks and Leaderboard are registry entries (`weather`) +// whose manifests declare another id (`ledmatrix-weather`). The plugin +// list, its config section and /plugins/toggle know them by that id, but +// the button enabled the registry id: /plugins/toggle answered 404 +// "Plugin not found" and the plugin stayed disabled behind "installed, +// but enabling it failed". It now enables the id the install answer +// names (`plugin_id`), or, from an answer without one, the installed +// entry the store entry matches (its id, plugin_path name or aliases). +// +// 2. Reinstall (the same button on an installed plugin) enabled it too, so +// reinstalling a plugin the user had switched off switched it back on. +// Only a fresh install enables. +// +// Runs the whole of plugins_manager.js in the sandbox against a fake API. + +const { create } = require('../plugins_manager_sandbox'); + +let pass = 0, fail = 0; +const ok = (label, cond, extra) => cond + ? (pass++, console.log(' ok ' + label)) + : (fail++, console.log(' FAIL ' + label + (extra !== undefined ? ' -> ' + JSON.stringify(extra).slice(0, 400) : ''))); + +const STORE = [ + { id: 'weather', name: 'Weather', category: 'weather', plugin_path: 'plugins/ledmatrix-weather', + aliases: ['ledmatrix-weather'] }, + { id: 'clock-simple', name: 'Clock', category: 'time', plugin_path: 'plugins/clock-simple', aliases: [] }, +]; + +// A server with one install in flight. `queue` false answers the install +// directly; `names` false leaves plugin_id out of the answer (an older +// server); `installsAs` is the id the installed manifest declares. +function server({ installed = [], queue = true, names = true, installsAs }) { + const state = { installed: installed.map(p => ({ ...p })), polls: 0 }; + const done = (id) => { + if (!state.installed.some(p => p.id === installsAs)) { + state.installed.push({ id: installsAs, name: id, enabled: false }); + } + const result = { success: true, message: `Plugin ${id} installed successfully`, restart_required: false }; + if (names) result.plugin_id = installsAs; + return result; + }; + state.route = (method, url, body) => { + if (url.startsWith('/api/v3/plugins/store/list')) { + return { json: { status: 'success', data: { plugins: STORE } } }; + } + if (url.startsWith('/api/v3/plugins/installed')) { + return { json: { status: 'success', data: { plugins: state.installed.map(p => ({ ...p })) } } }; + } + if (method === 'POST' && url === '/api/v3/plugins/install') { + if (queue) return { json: { status: 'success', message: 'queued', data: { operation_id: 'op-1' } } }; + return { json: { status: 'success', message: 'Plugin installed successfully', ...done(body.plugin_id) } }; + } + if (url === '/api/v3/plugins/operation/op-1') { + state.polls++; + if (state.polls < 3) return { json: { status: 'success', data: { status: 'running' } } }; + return { json: { status: 'success', data: { status: 'completed', result: done('weather') } } }; + } + if (method === 'POST' && url === '/api/v3/plugins/toggle') { + const plugin = state.installed.find(p => p.id === body.plugin_id); + if (!plugin) return { status: 404, json: { status: 'error', message: 'Plugin not found' } }; + plugin.enabled = body.enabled; + return { json: { status: 'success', message: `Plugin ${body.plugin_id} enabled successfully` } }; + } + return { json: { status: 'success' } }; + }; + return state; +} + +async function install(pluginId, opts) { + const srv = server(opts); + const sb = create({ route: srv.route }); + sb.window.searchPluginStore(); + await sb.until(() => sb.requests.some(r => r.url.startsWith('/api/v3/plugins/store/list')), 'store list'); + await sb.window.pluginManager.loadInstalledPlugins(true); + await sb.settle(); + sb.requests.length = 0; + sb.toasts.length = 0; + sb.window.installPlugin(pluginId); + await sb.until(() => sb.toasts.some(t => /installed and enabled|enabling it failed|reinstalled/.test(t.message)), + 'the install to finish'); + await sb.settle(); + const toggles = sb.requests.filter(r => r.url === '/api/v3/plugins/toggle').map(r => r.body); + return { sb, srv, toggles }; +} + +(async () => { + console.log('\n1. a fresh install enables the id the plugin was installed as'); + { + const { srv, toggles, sb } = await install('weather', { installsAs: 'ledmatrix-weather' }); + ok('enables ledmatrix-weather, not the registry id', + JSON.stringify(toggles) === JSON.stringify([{ plugin_id: 'ledmatrix-weather', enabled: true }]), toggles); + ok('...which the server enabled', srv.installed.find(p => p.id === 'ledmatrix-weather').enabled === true, srv.installed); + ok('says so', sb.toasts.some(t => t.type === 'success' && /installed and enabled/.test(t.message)), sb.toasts); + ok('no "Plugin not found"', !sb.toasts.some(t => /not found|failed/.test(t.message)), sb.toasts); + const lastList = sb.requests.map(r => r.url).lastIndexOf('/api/v3/plugins/installed'); + const toggleAt = sb.requests.findIndex(r => r.url === '/api/v3/plugins/toggle'); + ok('the installed list is reloaded before enabling, so the new card is there to update', + lastList >= 0 && lastList < toggleAt, sb.requests.map(r => r.method + ' ' + r.url)); + } + { + const { toggles } = await install('weather', { installsAs: 'ledmatrix-weather', names: false }); + ok('an answer without plugin_id: the installed entry the store entry matches (its alias)', + JSON.stringify(toggles) === JSON.stringify([{ plugin_id: 'ledmatrix-weather', enabled: true }]), toggles); + } + { + const { toggles } = await install('weather', { installsAs: 'ledmatrix-weather', queue: false }); + ok('without the operation queue, from the direct answer', + JSON.stringify(toggles) === JSON.stringify([{ plugin_id: 'ledmatrix-weather', enabled: true }]), toggles); + } + { + const { toggles } = await install('clock-simple', { installsAs: 'clock-simple', names: false }); + ok('a plugin installed under its registry id is enabled by that id', + JSON.stringify(toggles) === JSON.stringify([{ plugin_id: 'clock-simple', enabled: true }]), toggles); + } + + console.log('\n2. a reinstall leaves the plugin as the user had it'); + { + const { srv, toggles, sb } = await install('weather', { + installsAs: 'ledmatrix-weather', installed: [{ id: 'ledmatrix-weather', name: 'Weather', enabled: false }], + }); + ok('sends no toggle', toggles.length === 0, toggles); + ok('the plugin stays disabled', srv.installed.find(p => p.id === 'ledmatrix-weather').enabled === false); + ok('says it was reinstalled', sb.toasts.some(t => t.type === 'success' && /reinstalled/.test(t.message)), sb.toasts); + ok('and reloads the list', + sb.requests.some(r => r.url === '/api/v3/plugins/installed'), sb.requests.map(r => r.url)); + } + { + const { toggles } = await install('weather', { + installsAs: 'ledmatrix-weather', installed: [{ id: 'ledmatrix-weather', name: 'Weather', enabled: true }], + }); + ok('an enabled plugin is not toggled either', toggles.length === 0, toggles); + } + + console.log(`\n${pass} passed, ${fail} failed`); + process.exit(fail ? 1 : 0); +})().catch(e => { console.error(e); process.exit(1); }); diff --git a/test/js/unit/test_store_registry_fields.js b/test/js/unit/test_store_registry_fields.js index 54a57ec5..968cd35f 100644 --- a/test/js/unit/test_store_registry_fields.js +++ b/test/js/unit/test_store_registry_fields.js @@ -52,7 +52,8 @@ global.installedPlugins = []; // eslint-disable-next-line no-eval eval([ 'function escapeHtml(text) {', 'function escapeAttribute(text) {', 'function jsStringAttr(value) {', - 'function isStorePluginInstalled(pluginIdOrPlugin) {', 'function renderPluginStore(plugins) {', + 'function isStorePluginInstalled(pluginIdOrPlugin) {', + 'function findInstalledStorePlugin(pluginIdOrPlugin) {', 'function renderPluginStore(plugins) {', ].map(extract).join('\n') + '\nglobal.renderPluginStore = renderPluginStore;' + '\nglobal.isStorePluginInstalled = isStorePluginInstalled;'); diff --git a/test/js/unit/test_update_all.js b/test/js/unit/test_update_all.js index ef0cf610..b3fbaa79 100644 --- a/test/js/unit/test_update_all.js +++ b/test/js/unit/test_update_all.js @@ -52,7 +52,7 @@ function fakeApi(behaviour = {}) { }; } -function setup(api, { stateList, windowList } = {}) { +function setup(api, { stateList, windowList, pluginManager } = {}) { global.window = { PluginAPI: api, installedPlugins: windowList, @@ -60,6 +60,7 @@ function setup(api, { stateList, windowList } = {}) { installedPlugins: stateList, loadInstalledPlugins: async () => stateList, }, + pluginManager, }; } @@ -92,12 +93,68 @@ const noSleep = { sleep: async () => {} }; ok('progress total counts only what is sent', progress.length === EXPECTED.length && progress.every(([, n]) => n === EXPECTED.length), progress); } + { + // A page without the plugin manager has no window.installedPlugins. + const api = fakeApi(); + setup(api, { stateList: INSTALLED }); + await Manager.updateAll(null, noSleep); + ok('the PluginStateManager list (no live list) is filtered the same way', + JSON.stringify(api.calls) === JSON.stringify(EXPECTED), api.calls); + } + + console.log('\na second run sends the live list, not the first run\'s snapshot'); + { + // Run 1 leaves PluginStateManager holding a, b, c. Then c is uninstalled + // and d installed: plugins_manager.js publishes that only as + // window.installedPlugins. Run 2 used to send a, b, c -- c failed as + // "plugin not found" and d, which had an update waiting, was skipped. + const api = fakeApi({ + c: () => { throw { error_code: 'PLUGIN_UPDATE_FAILED', message: 'Plugin update failed: plugin not found' }; }, + }); + const stale = [{ id: 'a' }, { id: 'b' }, { id: 'c' }]; + setup(api, { stateList: stale, windowList: [{ id: 'a' }, { id: 'b' }, { id: 'd' }] }); + const results = await Manager.updateAll(null, noSleep); + ok('sends exactly what is installed now', + JSON.stringify(api.calls) === JSON.stringify(['a', 'b', 'd']), api.calls); + ok('...so nothing fails over an uninstalled plugin', results.every(r => r.success), results); + } { const api = fakeApi(); setup(api, { stateList: INSTALLED, windowList: [] }); + const results = await Manager.updateAll(null, noSleep); + ok('an empty live list means nothing is installed: nothing is sent', + api.calls.length === 0 && results.length === 0, api.calls); + } + + console.log('\nthe end-of-run refresh redraws the installed grid'); + { + // PluginStateManager's refresh replaced window.installedPlugins and + // nothing else: the cards kept "Update to vX" and the Updates badge + // kept its count. The plugin manager's load renders the grid. + const loads = []; + let stateLoads = 0; + const pluginManager = { loadInstalledPlugins: async (force) => { loads.push(force); } }; + setup(fakeApi(), { stateList: INSTALLED, windowList: INSTALLED, pluginManager }); + window.PluginStateManager.loadInstalledPlugins = async () => { stateLoads++; }; await Manager.updateAll(null, noSleep); - ok('the PluginStateManager list is filtered the same way', - JSON.stringify(api.calls) === JSON.stringify(EXPECTED), api.calls); + ok('reloads through the plugin manager once, forced past its caches', + JSON.stringify(loads) === JSON.stringify([true]), loads); + ok('...instead of PluginStateManager', stateLoads === 0, stateLoads); + } + { + const pluginManager = { loadInstalledPlugins: async () => { throw new Error('offline'); } }; + const answer = { status: 'success', data: { update_status: 'updated' }, restart_required: true }; + setup(fakeApi({ 'ledmatrix-flights': () => answer }), { windowList: INSTALLED, pluginManager }); + const warn = console.warn; + console.warn = () => {}; + let results; + try { + results = await Manager.updateAll(null, noSleep); + } finally { + console.warn = warn; + } + ok('a failed plugin-manager reload still returns the results with their restart flag', + Array.isArray(results) && Manager.restartRequest(results) === answer); } { const api = fakeApi(); diff --git a/test/test_api_v3_install_reports_installed_id.py b/test/test_api_v3_install_reports_installed_id.py new file mode 100644 index 00000000..3e50af80 --- /dev/null +++ b/test/test_api_v3_install_reports_installed_id.py @@ -0,0 +1,104 @@ +"""POST /plugins/install says which id the plugin was installed as. + +A registry entry can install under another id: `weather` (aliases +`ledmatrix-weather`) installs a directory whose manifest declares +`ledmatrix-weather`, and that is the id the plugin list, the plugin's config +section and /plugins/toggle know it by. The store's Install button enabled +the new plugin by the registry id, which /plugins/toggle answered with 404 +"Plugin not found", so Weather, Music, Stocks and Leaderboard installed +disabled behind an "enabling it failed" warning. + +The answer -- the queued operation's result, or the direct response -- +carries `plugin_id`: the id the installed manifest declares, found the way +the store's update and uninstall find an install. +""" + +import json +from unittest.mock import MagicMock + +import pytest + +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401 + +INSTALL = "/api/v3/plugins/install" + + +@pytest.fixture +def store(api_v3_module, tmp_path): + manager = api_v3_module.api_v3.plugin_store_manager + manager.install_plugin.return_value = True + manager.get_registry_info.return_value = None + manager._find_plugin_path.return_value = None + + def installed_as(directory, manifest): + path = tmp_path / directory + path.mkdir() + (path / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8") + manager._find_plugin_path.side_effect = ( + lambda pid: path if pid == "weather" else None) + return path + + manager.installed_as = installed_as + return manager + + +@pytest.fixture +def queued(api_v3_module): + queue = MagicMock() + + def enqueue(operation_type, plugin_id, operation_callback=None): + queue.callback_result = operation_callback(MagicMock()) + return "op-1" + + queue.enqueue_operation.side_effect = enqueue + api_v3_module.api_v3.operation_queue = queue + return queue + + +class TestDirectInstall: + def test_an_aliased_entry_reports_the_manifest_id(self, api_v3_client, store): + store.installed_as("ledmatrix-weather", {"id": "ledmatrix-weather"}) + body = api_v3_client.post(INSTALL, json={"plugin_id": "weather"}).get_json() + assert body["status"] == "success" + assert body["plugin_id"] == "ledmatrix-weather" + store._find_plugin_path.assert_called_with("weather") + + def test_an_entry_installed_under_its_own_id_reports_that(self, api_v3_client, store): + store.installed_as("weather", {"id": "weather"}) + body = api_v3_client.post(INSTALL, json={"plugin_id": "weather"}).get_json() + assert body["plugin_id"] == "weather" + + def test_an_install_that_cannot_be_found_reports_the_requested_id(self, api_v3_client, store): + body = api_v3_client.post(INSTALL, json={"plugin_id": "weather"}).get_json() + assert body["status"] == "success" + assert body["plugin_id"] == "weather" + + def test_a_manifest_id_that_is_not_a_plain_name_is_not_passed_on(self, api_v3_client, store): + store.installed_as("ledmatrix-weather", {"id": "../elsewhere"}) + body = api_v3_client.post(INSTALL, json={"plugin_id": "weather"}).get_json() + assert body["plugin_id"] == "weather" + + def test_an_unreadable_manifest_reports_the_requested_id(self, api_v3_client, store): + path = store.installed_as("ledmatrix-weather", {}) + (path / "manifest.json").write_text("[not json", encoding="utf-8") + body = api_v3_client.post(INSTALL, json={"plugin_id": "weather"}).get_json() + assert body["plugin_id"] == "weather" + + def test_the_restart_fields_are_still_sent(self, api_v3_client, store): + store.installed_as("ledmatrix-weather", {"id": "ledmatrix-weather"}) + body = api_v3_client.post(INSTALL, json={"plugin_id": "weather"}).get_json() + assert "restart_required" in body + + +class TestQueuedInstall: + def test_the_operation_result_names_the_manifest_id(self, api_v3_client, store, queued): + store.installed_as("ledmatrix-weather", {"id": "ledmatrix-weather"}) + body = api_v3_client.post(INSTALL, json={"plugin_id": "weather"}).get_json() + assert body["data"]["operation_id"] == "op-1" + assert queued.callback_result["success"] is True + assert queued.callback_result["plugin_id"] == "ledmatrix-weather" + + def test_an_install_that_cannot_be_found_names_the_requested_id( + self, api_v3_client, store, queued): + api_v3_client.post(INSTALL, json={"plugin_id": "weather"}) + assert queued.callback_result["plugin_id"] == "weather" diff --git a/test/test_api_v3_installed_display_modes.py b/test/test_api_v3_installed_display_modes.py new file mode 100644 index 00000000..439fc86f --- /dev/null +++ b/test/test_api_v3_installed_display_modes.py @@ -0,0 +1,59 @@ +"""GET /api/v3/plugins/installed carries each plugin's ``display_modes``. + +The on-demand modal (plugins_manager.js) fills its Display Mode list from +``plugin.display_modes``, but the route never included the field, so every +plugin offered one option -- its own id -- under "This plugin exposes a +single display mode". The display turns that id into the plugin's first +mode, so a multi-mode plugin could only be started, and pinned, on that one. + +The modes come from the plugin catalog (the manifests the web process +discovered), the same source /display/modes and on-demand/start use. +""" + +from unittest.mock import MagicMock + +import pytest + +from test._api_v3_test_helpers import ( # noqa: F401 - fixtures + api_v3_client, api_v3_module, +) + + +@pytest.fixture +def installed(api_v3_module, api_v3_client, tmp_path): + def _get(declared_modes): + api = api_v3_module.api_v3 + # The listing's own metadata says nothing about modes: what the + # route reports must come from the catalog. + info = {'id': 'football-scoreboard', 'name': 'Football', 'version': '1.0.0'} + api.plugin_catalog.plugins_dir = str(tmp_path) # no manifest on disk + api.plugin_catalog.get_all_plugin_info = MagicMock(return_value=[info]) + api.plugin_catalog.get_plugin_display_modes = MagicMock(return_value=declared_modes) + api.plugin_store_manager.get_registry_info = MagicMock(return_value=None) + api.config_manager.load_config = MagicMock(return_value={}) + response = api_v3_client.get('/api/v3/plugins/installed') + assert response.status_code == 200 + plugins = [p for p in response.get_json()['data']['plugins'] + if p['id'] == 'football-scoreboard'] + assert len(plugins) == 1 + api.plugin_catalog.get_plugin_display_modes.assert_any_call('football-scoreboard') + return plugins[0] + return _get + + +def test_every_declared_mode_is_listed_in_order(installed): + modes = ['nfl_live', 'nfl_recent', 'nfl_upcoming'] + assert installed(modes)['display_modes'] == modes + + +def test_a_single_mode_plugin_lists_its_one_mode(installed): + assert installed(['clock-simple'])['display_modes'] == ['clock-simple'] + + +def test_no_declared_modes_is_an_empty_list(installed): + # The modal falls back to the plugin id for an empty list. + assert installed([])['display_modes'] == [] + + +def test_a_hand_edited_manifest_cannot_put_non_strings_in_the_list(installed): + assert installed(['nfl_live', 7, None, {'x': 1}])['display_modes'] == ['nfl_live'] diff --git a/test/test_async_plugin_updates.py b/test/test_async_plugin_updates.py index b908749f..9a23e5d3 100644 --- a/test/test_async_plugin_updates.py +++ b/test/test_async_plugin_updates.py @@ -13,6 +13,7 @@ The invariants that keep this change safe: inline path exactly. """ +import asyncio import os import sys import threading @@ -209,6 +210,32 @@ class TestFailurePaths: assert pm.get_plugin_lock(plugin_id).acquire(blocking=False) is True pm.get_plugin_lock(plugin_id).release() + @pytest.mark.parametrize("raised", [asyncio.CancelledError, SystemExit]) + def test_update_raising_a_base_exception_still_releases_the_plugin(self, pm, raised): + """asyncio.CancelledError and SystemExit derive from BaseException, + not Exception. Raised from update() on the worker, one skipped the + bookkeeping entirely: the plugin kept its lock and stayed RUNNING for + the life of the process -- never updated again, and every display() + skipped as busy.""" + class CancellingPlugin(SlowPlugin): + def update(self): + self.update_calls += 1 + raise raised() + + plugin_id = _install(pm, CancellingPlugin()) + pm.run_scheduled_updates() + deadline = time.monotonic() + 3 + while pm.plugins[plugin_id].update_calls == 0 and time.monotonic() < deadline: + time.sleep(0.05) + time.sleep(0.2) + + assert pm.get_plugin_lock(plugin_id).acquire(blocking=False) is True + pm.get_plugin_lock(plugin_id).release() + assert pm.state_manager.can_execute(plugin_id) is True + assert pm.plugin_last_update.get(plugin_id, 0) > 0 + error = pm.state_manager.get_error_info(plugin_id) + assert error is not None and error["error_type"] == raised.__name__ + def test_unloaded_while_queued_is_harmless(self, pm): """Exercise the public unload_plugin() lifecycle rather than deleting pm.plugins directly: queue the target's update behind a diff --git a/test/test_cache_long_keys.py b/test/test_cache_long_keys.py new file mode 100644 index 00000000..22f4714f --- /dev/null +++ b/test/test_cache_long_keys.py @@ -0,0 +1,99 @@ +"""A cache key too long to be a filename still gets a cache file. + +The calendar plugin's key joins every calendar id the user picked; on a real +install it passed 300 bytes, and since ext4 caps a filename at 255 every write +failed with ENAMETOOLONG -- logged as "permission denied", every update. +""" + +import logging +import os +from unittest.mock import patch + +from src.cache.disk_cache import DiskCache, _MAX_KEY_FILENAME_BYTES, _filename_stem +from src.cache_manager import CacheManager + +# The shape of the key that failed on hdpi, ids anonymised. +CALENDAR_KEY = ( + "calendar_events_someone@example.com_en.usa#holiday@group.v.calendar.google.com_" + "family13997378751670666433@group.calendar.google.com_ncaaf_-m-07kbp5_" + "%47eorgia+%42ulldogs+football#sports@group.v.calendar.google.com_nfl_-m-07l24_" + "%54ampa+%42ay+%42uccaneers#sports@group.v.calendar.google.com_primary" +) + +# ext4/xfs/btrfs NAME_MAX; set()'s temp file adds 15 bytes to the stem. +NAME_MAX = 255 +TEMP_OVERHEAD = len(".") + len(".json") + len(".") + 8 + + +def test_the_real_key_was_too_long_to_write(): + assert len((CALENDAR_KEY + ".json").encode()) > NAME_MAX - 10 + + +def test_a_long_key_round_trips(tmp_path): + cache = DiskCache(str(tmp_path)) + cache.set(CALENDAR_KEY, {"events": [1, 2, 3]}) + + assert cache.get(CALENDAR_KEY, max_age=None) == {"events": [1, 2, 3]} + path = cache.get_cache_path(CALENDAR_KEY) + assert os.path.isfile(path) + stem = os.path.basename(path)[:-len(".json")] + assert len(stem.encode()) + TEMP_OVERHEAD <= NAME_MAX + + +def test_short_keys_keep_their_filename(tmp_path): + cache = DiskCache(str(tmp_path)) + exactly = "k" * _MAX_KEY_FILENAME_BYTES + assert cache.get_cache_path("weather_current") == str(tmp_path / "weather_current.json") + assert cache.get_cache_path(exactly) == str(tmp_path / f"{exactly}.json") + assert cache.get_cache_path(exactly + "k") != str(tmp_path / f"{exactly}k.json") + + +def test_long_keys_sharing_a_prefix_stay_apart(tmp_path): + cache = DiskCache(str(tmp_path)) + first, second = CALENDAR_KEY + "_a", CALENDAR_KEY + "_b" + cache.set(first, {"which": "a"}) + cache.set(second, {"which": "b"}) + + assert cache.get_cache_path(first) != cache.get_cache_path(second) + assert cache.get(first, max_age=None) == {"which": "a"} + assert cache.get(second, max_age=None) == {"which": "b"} + + +def test_the_prefix_never_splits_a_character(): + key = "news_" + "é" * 300 # two bytes each, so the cut lands mid-character + stem = _filename_stem(key) + + assert stem.startswith("news_é") + assert len(stem.encode("utf-8")) <= _MAX_KEY_FILENAME_BYTES + stem.encode("utf-8").decode("utf-8") # well-formed + + +def test_a_stem_listed_by_the_web_ui_deletes_the_same_file(tmp_path): + with patch('src.cache_manager.CacheManager._get_writable_cache_dir', return_value=str(tmp_path)): + manager = CacheManager() + try: + manager.save_cache(CALENDAR_KEY, {"events": []}) + + listed = [entry["key"] for entry in manager.list_cache_files()] + assert len(listed) == 1 + manager.clear_cache(listed[0]) + + assert [n for n in os.listdir(tmp_path) if n.endswith(".json")] == [] + finally: + manager.stop_cleanup_thread() + + +def test_a_failed_write_names_the_real_error(tmp_path, monkeypatch, caplog): + blocker = tmp_path / "a-file" + blocker.write_text("") + # No writable fallback either, so set() gives up and says why. + monkeypatch.setattr(os.path, "expanduser", lambda _p: str(blocker / "home")) + cache = DiskCache(str(tmp_path / "missing")) + + with caplog.at_level(logging.WARNING): + cache.set("weather_current", {"t": 1}) + + gave_up = [r.getMessage() for r in caplog.records if "Could not write cache" in r.getMessage()] + assert len(gave_up) == 1 + assert "permission denied" not in gave_up[0] + assert os.strerror(2) in gave_up[0] # ENOENT: the directory does not exist diff --git a/test/test_cache_memory_tier_age.py b/test/test_cache_memory_tier_age.py new file mode 100644 index 00000000..ca53df90 --- /dev/null +++ b/test/test_cache_memory_tier_age.py @@ -0,0 +1,95 @@ +"""The memory tier never serves a record older than the reader asked for. + +A record read from disk went into the memory tier timed from the read, not +from when it was written, so get(max_age=300) could hand out data up to twice +that old: after a restart, after the memory sweep, or in a second process that +loaded a record once and kept serving it. +""" + +import time +from unittest.mock import patch + +import pytest + +from src.cache_manager import CacheManager + + +class Clock: + def __init__(self, now): + self.now = now + + def __call__(self): + return self.now + + +@pytest.fixture +def clock(monkeypatch): + fake = Clock(1_800_000_000.0) + monkeypatch.setattr(time, "time", fake) + return fake + + +def _manager(path): + # No disk sweep: it judges files by their real mtime against the fake + # clock and would delete them as months old. + with patch('src.cache_manager.CacheManager._get_writable_cache_dir', + return_value=str(path)), \ + patch('src.cache_manager.CacheManager.start_cleanup_thread'): + return CacheManager() + + +def test_a_record_loaded_late_expires_on_its_own_timestamp(tmp_path, clock): + writer = _manager(tmp_path) + writer.set("weather_current", {"t": 1}) + + reader = _manager(tmp_path) # a restart, or the other process + clock.now += 250 + assert reader.get("weather_current", max_age=300) == {"t": 1} + + clock.now += 100 # the data is 350 s old; it sat in memory for 100 s + assert reader.get("weather_current", max_age=300) is None + + +def test_a_stored_ttl_bounds_the_memory_copy_too(tmp_path, clock): + writer = _manager(tmp_path) + writer.set("odds_espn_football_nfl_401", {"spread": 6.5}, ttl=60) + + reader = _manager(tmp_path) + clock.now += 55 + assert reader.get("odds_espn_football_nfl_401", max_age=3600) == {"spread": 6.5} + + clock.now += 60 + assert reader.get("odds_espn_football_nfl_401", max_age=3600) is None + + +def test_a_stale_memory_copy_gives_way_to_a_newer_write_on_disk(tmp_path, clock): + writer = _manager(tmp_path) + reader = _manager(tmp_path) + writer.set("stocks_AAPL", {"price": 1}) + assert reader.get("stocks_AAPL", max_age=300) == {"price": 1} + + clock.now += 280 + writer.set("stocks_AAPL", {"price": 2}) + clock.now += 40 # reader's copy: 40 s in memory, 320 s old + + assert reader.get("stocks_AAPL", max_age=300) == {"price": 2} + + +def test_fresh_records_are_still_served_from_memory(tmp_path, clock): + manager = _manager(tmp_path) + manager.set("news_NFL", {"items": []}) + clock.now += 100 + + with patch.object(manager._disk_cache_component, "get") as disk_get: + assert manager.get("news_NFL", max_age=300) == {"items": []} + disk_get.assert_not_called() + + +def test_max_age_none_and_records_without_a_timestamp_never_expire(tmp_path, clock): + manager = _manager(tmp_path) + manager.set("plugin_health_x", {"ok": True}) + manager.save_cache("raw_record", {"no": "timestamp"}) + clock.now += 10 ** 6 + + assert manager.get("plugin_health_x", max_age=None) == {"ok": True} + assert manager.get_cached_data("raw_record", max_age=None) == {"no": "timestamp"} diff --git a/test/test_config_service_notify.py b/test/test_config_service_notify.py new file mode 100644 index 00000000..cf687520 --- /dev/null +++ b/test/test_config_service_notify.py @@ -0,0 +1,199 @@ +"""ConfigService notifies subscribers outside its lock, in order. + +Subscribers ran while _load_config held the service's lock. The display's +per-plugin subscriber is PluginManager.apply_config_change, which waits up to +PLUGIN_LOCK_TIMEOUT (5 s) for a busy plugin. The same save that toggles a +plugin's ``enabled`` flags a reconcile, and the render thread runs it: its +get_config() -- and the unsubscribe() of a plugin it disables -- waited behind +every slow callback, freezing the panel for up to 5 s per busy plugin. + +What callers could rely on before still holds: one reload's notifications +finish before the next reload's start, and a callback unsubscribe() removed is +not running, and will not run, once unsubscribe() returns. +""" + +import itertools +import json +import os +import threading +import time + +import pytest + +from src.config_manager import ConfigManager +from src.config_service import ConfigService + +SLOW = 2.0 # how long a blocked callback waits before giving up + + +@pytest.fixture +def service(tmp_path): + config_path = tmp_path / "config.json" + config_path.write_text(json.dumps({"display": {"brightness": 50}, + "weather": {"enabled": True}}), + encoding="utf-8") + manager = ConfigManager(str(config_path), str(tmp_path / "config_secrets.json")) + manager.template_path = str(tmp_path / "no-template.json") + svc = ConfigService(manager, enable_hot_reload=False) + yield svc, config_path + svc.shutdown() + + +_saves = itertools.count(1) + + +def _save(config_path, **sections): + config = json.loads(config_path.read_text(encoding="utf-8")) + config.update(sections) + config_path.write_text(json.dumps(config), encoding="utf-8") + # ConfigManager re-reads only when (mtime, size) moves. Two quick saves of + # the same size can share an mtime tick (about 16 ms on Windows), so step + # it forward explicitly. + st = config_path.stat() + os.utime(config_path, ns=(st.st_atime_ns, st.st_mtime_ns + next(_saves) * 50_000_000)) + + +def _reload_in_background(svc): + thread = threading.Thread(target=svc._load_config, daemon=True) + thread.start() + return thread + + +def test_get_config_does_not_wait_for_a_slow_subscriber(service): + svc, config_path = service + entered, release = threading.Event(), threading.Event() + + def slow(_old, _new): + entered.set() + release.wait(SLOW) + + svc.subscribe(slow, plugin_id="weather") + _save(config_path, weather={"enabled": False}) + reload = _reload_in_background(svc) + assert entered.wait(SLOW) + + start = time.monotonic() + config = svc.get_config() + waited = time.monotonic() - start + release.set() + reload.join(SLOW) + + assert waited < 0.5 + # Swapped before anyone was told: a subscriber that reads it sees the new one. + assert config["weather"]["enabled"] is False + + +def test_unsubscribing_another_callback_does_not_wait(service): + svc, config_path = service + entered, release = threading.Event(), threading.Event() + + def slow(_old, _new): + entered.set() + release.wait(SLOW) + + def other(_old, _new): + pass + + svc.subscribe(slow, plugin_id="weather") + svc.subscribe(other, plugin_id="clock") + _save(config_path, weather={"enabled": False}) + reload = _reload_in_background(svc) + assert entered.wait(SLOW) + + start = time.monotonic() + svc.unsubscribe(other, plugin_id="clock") + waited = time.monotonic() - start + release.set() + reload.join(SLOW) + + assert waited < 0.5 + + +def test_a_callback_unsubscribed_mid_notification_is_not_called(service): + svc, config_path = service + entered, release = threading.Event(), threading.Event() + called = [] + + def slow_global(_old, _new): # global subscribers are notified first + entered.set() + release.wait(SLOW) + + def weather(_old, _new): + called.append("weather") + + svc.subscribe(slow_global) + svc.subscribe(weather, plugin_id="weather") + _save(config_path, weather={"enabled": False}) + reload = _reload_in_background(svc) + assert entered.wait(SLOW) + + svc.unsubscribe(weather, plugin_id="weather") + release.set() + reload.join(SLOW) + + assert called == [] + + +def test_unsubscribe_waits_for_its_own_callback_to_return(service): + svc, config_path = service + entered, release = threading.Event(), threading.Event() + returned = threading.Event() + + def slow(_old, _new): + entered.set() + release.wait(SLOW) + returned.set() + + svc.subscribe(slow, plugin_id="weather") + _save(config_path, weather={"enabled": False}) + reload = _reload_in_background(svc) + assert entered.wait(SLOW) + + threading.Timer(0.2, release.set).start() + svc.unsubscribe(slow, plugin_id="weather") + + assert returned.is_set() + reload.join(SLOW) + + +def test_a_callback_may_read_config_and_unsubscribe_itself(service): + svc, config_path = service + seen = [] + + def once(_old, _new): + seen.append(svc.get_config()["weather"]["enabled"]) + svc.unsubscribe(once, plugin_id="weather") + + svc.subscribe(once, plugin_id="weather") + _save(config_path, weather={"enabled": False}) + reload = _reload_in_background(svc) + reload.join(SLOW) + + assert not reload.is_alive() + assert seen == [False] + + +def test_two_reloads_notify_in_order(service): + svc, config_path = service + entered, release = threading.Event(), threading.Event() + seen = [] + + def record(old, new): + seen.append((old["brightness"], new["brightness"])) + if len(seen) == 1: + entered.set() + release.wait(SLOW) + + svc.subscribe(record, plugin_id="display") + _save(config_path, display={"brightness": 60}) + first = _reload_in_background(svc) + assert entered.wait(SLOW) + + _save(config_path, display={"brightness": 100}) + second = _reload_in_background(svc) + time.sleep(0.2) + release.set() + first.join(SLOW) + second.join(SLOW) + + assert seen == [(50, 60), (60, 100)] diff --git a/test/test_display_duration_not_a_number.py b/test/test_display_duration_not_a_number.py new file mode 100644 index 00000000..1d43497b --- /dev/null +++ b/test/test_display_duration_not_a_number.py @@ -0,0 +1,112 @@ +"""A plugin duration that is not a number must not stop the display. + +Several plugins return their ``display_duration`` setting as it is in +config.json (``return self.config.get('display_duration', 15.0)``), so a +value saved as ``"20"`` or ``null`` -- from the raw config editor, or by +hand -- reached run() as a string or None. _resolve_durations then compared +it with 0, the TypeError went past every handler in the loop, and the +display service exited; systemd restarted it into the same screen and the +same crash. +""" + +import logging +import math +import os +from unittest.mock import MagicMock + +os.environ.setdefault("EMULATOR", "true") + +import pytest + +from src.display_controller import DisplayController +from test._run_loop_harness import FakePlugin, RunLoopHarness + + +def _controller(plugin_modes): + dc = object.__new__(DisplayController) + dc.config = {} + dc.plugin_modes = plugin_modes + return dc + + +def _plugin(duration, plugin_id='clock-simple'): + plugin = MagicMock() + plugin.plugin_id = plugin_id + plugin.get_display_duration.return_value = duration + return plugin + + +class TestPluginDurationIsCoerced: + @pytest.mark.parametrize('value, expected', [ + ('20', 20.0), (' 7.5 ', 7.5), (12, 12.0), (12.5, 12.5)]) + def test_numbers_and_numeric_strings_are_used(self, value, expected): + dc = _controller({'clock': _plugin(value)}) + duration = dc._get_display_duration('clock') + assert duration == expected and isinstance(duration, float) + + @pytest.mark.parametrize('value', [ + None, '', 'twenty', True, False, float('nan'), float('inf'), 'inf', + [20], {'seconds': 20}]) + def test_anything_but_a_finite_number_gets_the_default(self, value): + dc = _controller({'clock': _plugin(value)}) + assert dc._get_display_duration('clock') == 30 + + @pytest.mark.parametrize('value', [0, -5, '-5', '0']) + def test_a_number_not_above_zero_still_gets_the_15s_rule(self, value): + """Unchanged: _resolve_durations turns it into 15 s, with its warning.""" + plugin = _plugin(value) + dc = _controller({'clock': plugin}) + base = dc._get_display_duration('clock') + assert dc._resolve_durations(plugin, 'clock', base, False)[1] == 15.0 + + def test_a_raising_get_display_duration_gets_the_default(self): + plugin = _plugin(None) + plugin.get_display_duration.side_effect = KeyError('display_duration') + assert _controller({'clock': plugin})._get_display_duration('clock') == 30 + + def test_the_result_feeds_resolve_durations(self): + """The two calls run() makes back to back, for one screen.""" + plugin = _plugin('bad') + dc = _controller({'clock': plugin}) + base = dc._get_display_duration('clock') + assert dc._resolve_durations(plugin, 'clock', base, False) == (30, 30) + + def test_logged_once_per_plugin(self, caplog): + dc = _controller({'clock': _plugin('twenty'), + 'clock_big': _plugin('twenty'), + 'calendar': _plugin(None, plugin_id='calendar')}) + # clock_big is a second mode of the same plugin. + dc.plugin_modes['clock_big'].plugin_id = 'clock-simple' + with caplog.at_level(logging.WARNING, logger='src.display_controller'): + for _ in range(3): + for mode in ('clock', 'clock_big', 'calendar'): + dc._get_display_duration(mode) + warnings = [r for r in caplog.records if 'display duration' in r.getMessage()] + assert len(warnings) == 2 + assert {'clock-simple', 'calendar'} == { + next(p for p in ('clock-simple', 'calendar') if p in r.getMessage()) + for r in warnings} + + def test_a_good_value_after_a_bad_one_is_used(self): + plugin = _plugin(None) + dc = _controller({'clock': plugin}) + assert dc._get_display_duration('clock') == 30 + plugin.get_display_duration.return_value = 45 + assert dc._get_display_duration('clock') == 45.0 + + +class TestRunLoopSurvives: + """Through the real run() on the harness's fake clock.""" + + @pytest.mark.parametrize('duration, shown_for', [('20', 20.0), (None, 30.0), + ('twenty', 30.0)]) + def test_the_screen_runs_and_the_rotation_goes_on(self, tmp_path, duration, shown_for): + harness = RunLoopHarness(tmp_path, horizon=120) + harness.add_plugin(FakePlugin("weather", ["weather"], duration=30)) + harness.add_plugin(FakePlugin("clock-simple", ["clock"], duration=duration)) + # Before the fix run() returned at t=30, when the clock came up, and + # the harness raised "run() returned ... before the horizon". + rows = harness.run()["screens"] + clock = next(row for row in rows if row[1] == "clock") + assert math.isclose(clock[2], shown_for, abs_tol=1.0) + assert [row[1] for row in rows][:3] == ["weather", "clock", "weather"] diff --git a/test/test_ipc_state_stream.py b/test/test_ipc_state_stream.py index d512d68c..5418a945 100644 --- a/test/test_ipc_state_stream.py +++ b/test/test_ipc_state_stream.py @@ -346,6 +346,52 @@ class TestSubscriptionStore: assert client.snapshot_loop_age(snap, now_mono=104.0) is None +class TestReconnectBackoff: + """StateSubscription._run's waits between connections, without a socket.""" + + def test_a_connection_that_got_a_snapshot_starts_the_backoff_over(self, hub, + monkeypatch): + """Three failed tries, then the display is back twice, restarting + each time, then gone again. Each restart is retried after the + shortest wait, not after whatever the waits had grown to.""" + sub = client.StateSubscription(paths=['/nowhere']) + script = ['refused', 'refused', 'refused', 'snapshot', 'snapshot', 'refused'] + waits = [] + + def follow(): + step = script.pop(0) + if step == 'snapshot': # subscribed, then the display restarted + sub._store(hub.snapshot(), full=True) + raise client.ControlError('closed', 'the display closed the connection') + raise client.ControlError(step) + + def wait(seconds): + waits.append(seconds) + return not script # True ends _run, as stop() would + + monkeypatch.setattr(sub, '_follow', follow) + monkeypatch.setattr(sub._stop, 'wait', wait) + sub._run() + first = client._RECONNECT_MIN_SECONDS + assert waits == [first, 2 * first, 4 * first, first, first, 2 * first] + + def test_a_display_without_the_stream_is_still_retried_slowly(self, monkeypatch): + sub = client.StateSubscription(paths=['/nowhere']) + waits = [] + + def follow(): + raise client.ControlError('unknown_command') + + def wait(seconds): + waits.append(seconds) + return len(waits) == 2 + + monkeypatch.setattr(sub, '_follow', follow) + monkeypatch.setattr(sub._stop, 'wait', wait) + sub._run() + assert waits == [client._RECONNECT_MAX_SECONDS] * 2 + + # --- a real socket ------------------------------------------------------------------ def _wait_until(predicate, timeout=5.0): diff --git a/test/test_plugin_loader_reload_isolation.py b/test/test_plugin_loader_reload_isolation.py index 014153d1..3c0dbdfe 100644 --- a/test/test_plugin_loader_reload_isolation.py +++ b/test/test_plugin_loader_reload_isolation.py @@ -58,3 +58,83 @@ def test_a_reloaded_plugin_still_gets_its_own_bare_module(plugins): assert reloaded.WHO == "alpha" assert sys.path.index(str(plugins["alpha"])) < sys.path.index(str(plugins["beta"])) assert sys.path.count(str(plugins["alpha"])) == 1 + + +# -- sub-packages ------------------------------------------------------------ +# +# A plugin that keeps helpers in a package (``providers/feed.py``, imported as +# ``from providers.feed import ...``) leaves dotted entries in sys.modules. +# Only the bare ``providers`` used to be tracked, so ``providers.feed`` outlived +# the plugin: a reload after a store update re-ran the new manager.py against +# the old feed.py, until the display restarted. Elections (providers/), +# flights (enrichment/) and olympics (data/, renderers/) ship packages. + + +@pytest.fixture +def package_plugin(tmp_path): + before_path = list(sys.path) + before_modules = set(sys.modules) + plugin_dir = tmp_path / "pkgdemo" + (plugin_dir / "providers").mkdir(parents=True) + (plugin_dir / "providers" / "__init__.py").write_text("", encoding="utf-8") + (plugin_dir / "providers" / "feed.py").write_text("VERSION = 'v1'\n", encoding="utf-8") + (plugin_dir / "manager.py").write_text( + "from providers.feed import VERSION\n", encoding="utf-8") + yield plugin_dir + sys.path[:] = before_path + for key in set(sys.modules) - before_modules: + sys.modules.pop(key, None) + + +def test_a_reloaded_plugin_runs_its_updated_subpackage_module(package_plugin): + loader = PluginLoader() + assert loader.load_module("pkgdemo", package_plugin, "manager.py").VERSION == "v1" + + _unload(loader, "pkgdemo") + # The store update: a different size, so no cached bytecode can match. + (package_plugin / "providers" / "feed.py").write_text( + "VERSION = 'v2 from the update'\n", encoding="utf-8") + reloaded = loader.load_module("pkgdemo", package_plugin, "manager.py") + + assert reloaded.VERSION == "v2 from the update" + + +def test_unload_drops_the_plugins_subpackage_modules(package_plugin): + loader = PluginLoader() + loader.load_module("pkgdemo", package_plugin, "manager.py") + # Still importable while the plugin runs, as before. + assert "providers.feed" in sys.modules + + _unload(loader, "pkgdemo") + + assert not [k for k in sys.modules if k.startswith("providers")] + + +def test_a_failed_load_leaves_no_subpackage_module_behind(package_plugin): + (package_plugin / "manager.py").write_text( + "from providers.feed import VERSION\nraise RuntimeError('broken')\n", + encoding="utf-8") + loader = PluginLoader() + + with pytest.raises(RuntimeError): + loader.load_module("pkgdemo", package_plugin, "manager.py") + + assert not [k for k in sys.modules if k.startswith("providers")] + + +def test_unload_leaves_packages_from_outside_the_plugin_alone(package_plugin, tmp_path): + # A library the plugin imports is not the plugin's to drop. + lib_root = tmp_path / "site" + (lib_root / "extlib").mkdir(parents=True) + (lib_root / "extlib" / "__init__.py").write_text("", encoding="utf-8") + (lib_root / "extlib" / "sub.py").write_text("X = 1\n", encoding="utf-8") + sys.path.append(str(lib_root)) + (package_plugin / "manager.py").write_text( + "import extlib.sub\nfrom providers.feed import VERSION\n", encoding="utf-8") + loader = PluginLoader() + loader.load_module("pkgdemo", package_plugin, "manager.py") + + _unload(loader, "pkgdemo") + + assert "extlib.sub" in sys.modules + assert "extlib" in sys.modules diff --git a/test/test_plugin_loader_symlinked_dir.py b/test/test_plugin_loader_symlinked_dir.py new file mode 100644 index 00000000..17a36244 --- /dev/null +++ b/test/test_plugin_loader_symlinked_dir.py @@ -0,0 +1,81 @@ +"""A dev plugin linked in under a name its checkout does not share still loads. + +``scripts/dev/dev_plugin_setup.sh`` links a checkout into the plugins +directory under the plugin's id: ``link-github foo `` clones +``ledmatrix-foo`` (the repository naming convention) and links it as +``plugins/foo``. ``contained_plugin_dir`` resolved the link and looked for the +*target's* folder name, ``ledmatrix-foo``, among the plugins directory's +entries. There is none, so ``install_dependencies`` refused the plugin as +outside the plugins directory and the load failed with "Dependency +installation failed" -- even with no requirements.txt at all. + +The containment it exists for still holds: the answer is always rebuilt from +an entry enumerated under the plugins directory. + +Skipped where this process cannot create a symlink (Windows without the +privilege). +""" + +import os +from unittest.mock import MagicMock, patch + +import pytest + +from src.plugin_system.plugin_loader import PluginLoader, contained_plugin_dir + + +def _symlink_or_skip(target, link): + try: + os.symlink(target, link, target_is_directory=True) + except (OSError, NotImplementedError) as e: + pytest.skip(f"cannot create a symlink here: {e}") + + +@pytest.fixture +def linked(tmp_path): + checkout = tmp_path / "dev-plugins" / "ledmatrix-foo" + checkout.mkdir(parents=True) + plugins_dir = tmp_path / "plugins" + plugins_dir.mkdir() + link = plugins_dir / "foo" + _symlink_or_skip(checkout, link) + return plugins_dir, link, checkout + + +def test_a_link_resolves_to_its_own_entry_in_the_plugins_dir(linked): + plugins_dir, link, _checkout = linked + + assert contained_plugin_dir(link, plugins_dir) == os.path.join( + os.path.realpath(plugins_dir), "foo") + + +def test_a_linked_plugin_without_requirements_needs_no_install(linked): + plugins_dir, link, _checkout = linked + + with patch("subprocess.run") as pip: + assert PluginLoader().install_dependencies(link, "foo", plugins_dir=plugins_dir) is True + pip.assert_not_called() + + +@patch("src.plugin_system.plugin_loader.requirements_are_satisfied", return_value=False) +def test_a_linked_plugins_requirements_are_installed_through_the_link(_satisfied, linked): + plugins_dir, link, checkout = linked + (checkout / "requirements.txt").write_text("package1==1.0.0\n", encoding="utf-8") + + with patch("subprocess.run", return_value=MagicMock(returncode=0, stderr="")) as pip: + assert PluginLoader().install_dependencies(link, "foo", plugins_dir=plugins_dir) is True + + argv = pip.call_args[0][0] + assert argv[argv.index("-r") + 1] == os.path.join( + os.path.realpath(plugins_dir), "foo", "requirements.txt") + + +def test_a_link_outside_the_plugins_dir_is_still_refused(linked, tmp_path): + plugins_dir, _link, checkout = linked + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + stray = elsewhere / "bar" + _symlink_or_skip(checkout, stray) + + assert contained_plugin_dir(stray, plugins_dir) is None + assert contained_plugin_dir(plugins_dir / ".." / "elsewhere" / "bar", plugins_dir) is None diff --git a/test/test_plugin_system.py b/test/test_plugin_system.py index 555f5891..3e4f7e7b 100644 --- a/test/test_plugin_system.py +++ b/test/test_plugin_system.py @@ -189,6 +189,24 @@ class TestPluginExecutor: assert result is False + def test_a_base_exception_is_a_failure_not_a_timeout(self): + """asyncio.CancelledError derives from BaseException. Uncaught on + the executor's thread it ended the thread with the call never marked + complete, so a call that failed at once was reported, and recorded, + as timing out.""" + import asyncio + import pytest + from src.exceptions import PluginError + from src.plugin_system.plugin_executor import PluginExecutor + executor = PluginExecutor(default_timeout=5.0) + + def cancelled(): + raise asyncio.CancelledError() + + with pytest.raises(PluginError) as raised: + executor.execute_with_timeout(cancelled, plugin_id="test_plugin") + assert isinstance(raised.value.__cause__, asyncio.CancelledError) + class TestPluginHealth: """Test plugin health monitoring.""" diff --git a/test/test_scroll_helper_narrow_strip.py b/test/test_scroll_helper_narrow_strip.py new file mode 100644 index 00000000..9373cb59 --- /dev/null +++ b/test/test_scroll_helper_narrow_strip.py @@ -0,0 +1,112 @@ +""" +ScrollHelper frames for a strip narrower than the panel, and other wraps. + +A frame that runs past the end of the strip continues from its head: column +j of the frame is strip column (position + j) modulo the strip's width. The +wrap path sliced the strip's tail and then "the rest of the frame" from its +head, which assumed the head was at least that wide. For a strip narrower +than the panel it raised ValueError at every position, so a narrow strip +(Vegas composes one when its content is narrower than the chain, with its +lead-in of 0) logged a traceback every frame instead of drawing. +""" + +import numpy as np +import pytest +from PIL import Image + +from src.common.scroll_helper import ScrollHelper + +W, H = 128, 32 + + +def _strip(width, height=H): + """A strip whose every column is distinct: R and B are the column number.""" + columns = np.arange(width) + pixels = np.zeros((height, width, 3), dtype=np.uint8) + pixels[:, :, 0] = columns % 256 + pixels[:, :, 1] = 255 - (columns % 256) + pixels[:, :, 2] = columns // 256 + return Image.fromarray(pixels, 'RGB') + + +def _helper(strip_width, sub_pixel=False): + sh = ScrollHelper(W, H) + sh.set_scrolling_image(_strip(strip_width)) + sh.sub_pixel_scrolling = sub_pixel + return sh + + +def _frame(sh, position): + sh.scroll_position = position + frame = sh.get_visible_portion() + assert frame is not None and frame.size == (W, H) and frame.mode == 'RGB' + return np.asarray(frame) + + +def _wrapped(sh, start): + """What the panel should show from ``start``: the strip, wrapping.""" + return sh.cached_array[:, np.arange(start, start + W) % sh.cached_array.shape[1]] + + +class TestNarrowStrip: + @pytest.mark.parametrize('strip_width', [1, 40, 50, W - 1]) + @pytest.mark.parametrize('position', [0, 10, 39]) + def test_frame_repeats_the_strip_across_the_panel(self, strip_width, position): + sh = _helper(strip_width) + position %= strip_width + assert np.array_equal(_frame(sh, position), _wrapped(sh, position)) + + def test_a_composed_strip_without_lead_in(self): + # How Vegas builds its strip: lead_gap=0 (vegas_scroll.lead_in_width). + sh = ScrollHelper(W, H) + sh.create_scrolling_image([_strip(40)], item_gap=0, element_gap=0, lead_gap=0) + assert sh.total_scroll_width == 40 + for position in range(40): + assert np.array_equal(_frame(sh, position), _wrapped(sh, position)) + + def test_a_whole_pass_scrolls_without_raising(self): + sh = _helper(50) + sh.set_pixels_per_frame(3) + for _ in range(60): + sh.update_scroll_position() + assert sh.get_visible_portion().size == (W, H) + + @pytest.mark.parametrize('position', [0.5, 10.25, 49.5]) + def test_sub_pixel_blend_of_a_narrow_strip(self, position): + sh = _helper(50, sub_pixel=True) + frame = _frame(sh, position) + start = int(position) + near, far = _wrapped(sh, start), _wrapped(sh, start + 1) + lo, hi = np.minimum(near, far), np.maximum(near, far) + assert (frame >= lo).all() and (frame <= hi).all() + + +class TestWrapOfAWideStrip: + """Unchanged: the tail, then the head.""" + + @pytest.mark.parametrize('position', [200 - W + 1, 150, 199]) + def test_tail_then_head(self, position): + sh = _helper(200) + frame = _frame(sh, position) + tail = 200 - position + assert np.array_equal(frame[:, :tail], sh.cached_array[:, position:]) + assert np.array_equal(frame[:, tail:], sh.cached_array[:, :W - tail]) + + def test_at_the_end_shows_the_head(self): + sh = _helper(200) + assert np.array_equal(_frame(sh, 200), sh.cached_array[:, :W]) + + def test_sub_pixel_at_the_last_column(self): + sh = _helper(200, sub_pixel=True) + assert _frame(sh, 199.5).shape == (H, W, 3) + + def test_a_position_before_the_start_wraps_too(self): + # Slicing [-10:118] of the array was an empty slice: frombytes raised. + sh = _helper(200) + assert np.array_equal(_frame(sh, -10), _wrapped(sh, -10)) + + +def test_a_zero_width_strip_is_a_black_frame(): + sh = ScrollHelper(W, H) + sh.set_scrolling_image(Image.new('RGB', (0, H))) + assert not _frame(sh, 0).any() diff --git a/test/test_sports_card.py b/test/test_sports_card.py index f8de86f5..cafde765 100644 --- a/test/test_sports_card.py +++ b/test/test_sports_card.py @@ -13,6 +13,8 @@ body. That is what let all eight adopt this with byte-identical renders. import logging import json import os +from datetime import datetime, timezone +from zoneinfo import ZoneInfo import pytest @@ -169,6 +171,95 @@ class TestDateAndTime: assert C.card_tzinfo({"timezone": "Not/AZone"}, log) is timezone.utc +class TestWeekdayMatchesThePrintedDate: + """The weekday is the printed date's, whichever zone printed it. + + The extractor prints "M/D" in the plugin's resolved zone (its own + setting, else the global one, else the system zone). The card is handed + only the plugin's config, whose ``timezone`` ships as "" -- so a weekday + taken in card_tzinfo's zone was UTC's, and an evening kickoff in the + Americas read "Sat Oct 2" for a Friday game. + """ + + WEEKDAY = {"timezone": "", "scroll_card": {"date_format": "weekday"}} + + @staticmethod + def _as_printed(start_utc, zone): + """The game dict and the date text, as the extractor builds them.""" + local = datetime.fromisoformat(start_utc).astimezone(ZoneInfo(zone)) + game = {"start_time_utc": datetime.fromisoformat(start_utc), + "game_date": f"{local.month}/{local.day}"} + want = f"{C.WEEKDAY_ABBR[local.weekday()]} {C.MONTH_ABBR[local.month - 1]} {local.day}" + return game, want + + @pytest.mark.parametrize("start_utc, zone, want", [ + # Friday 8 PM EDT is Saturday in UTC. + ("2026-10-03T00:00:00+00:00", "America/New_York", "Fri Oct 2"), + # The night US clocks go back: 8:30 PM EDT Saturday, then 11 PM EST + # Sunday, each the next day in UTC. + ("2026-11-01T00:30:00+00:00", "America/New_York", "Sat Oct 31"), + ("2026-11-02T04:00:00+00:00", "America/New_York", "Sun Nov 1"), + # New Year's Eve on the west coast is New Year's Day in UTC. + ("2027-01-01T04:00:00+00:00", "America/Los_Angeles", "Thu Dec 31"), + # Just east of the date line: Pago Pago's Friday evening. + ("2026-10-03T05:00:00+00:00", "Pacific/Pago_Pago", "Fri Oct 2"), + # Just west of it, the other way: Saturday morning in Auckland is + # Friday in UTC -- and the 10 AM game on the day NZ clocks go forward. + ("2026-10-02T20:00:00+00:00", "Pacific/Auckland", "Sat Oct 3"), + ("2026-09-26T21:00:00+00:00", "Pacific/Auckland", "Sun Sep 27"), + # UTC+14, the furthest any zone sits from UTC. + ("2026-10-02T11:00:00+00:00", "Pacific/Kiritimati", "Sat Oct 3"), + # A zone on UTC's own date needs nothing. + ("2026-10-02T19:00:00+00:00", "Europe/London", "Fri Oct 2"), + ]) + def test_the_shipped_blank_timezone(self, log, start_utc, zone, want): + game, printed = self._as_printed(start_utc, zone) + assert printed == want # the case says what the extractor prints + assert C.format_game_date(self.WEEKDAY, log, game["game_date"], game) == want + + def test_an_iso_string_start_reads_the_same(self, log): + game = {"start_time_utc": "2026-10-03T00:00:00Z"} + assert C.format_game_date(self.WEEKDAY, log, "10/2", game) == "Fri Oct 2" + assert C.format_game_date(self.WEEKDAY, log, "10/02", game) == "Fri Oct 2" + + def test_a_plugin_level_zone_still_agrees(self, log): + game, want = self._as_printed("2026-10-03T00:00:00+00:00", "America/Chicago") + cfg = dict(self.WEEKDAY, timezone="America/Chicago") + assert C.format_game_date(cfg, log, game["game_date"], game) == want == "Fri Oct 2" + + def test_a_date_no_zone_could_print_keeps_the_zone_weekday(self, log): + """More than a day from the start: nothing to anchor to, so the + weekday is card_tzinfo's, as it always was.""" + game = {"start_time_utc": datetime(2026, 10, 3, 0, 0, tzinfo=timezone.utc)} + assert C.format_game_date(self.WEEKDAY, log, "10/9", game) == "Sat Oct 9" + + def test_a_start_without_an_offset_keeps_the_zone_weekday(self, log): + """A naive time names no instant, so it cannot place the date.""" + game = {"start_time_utc": datetime(2026, 10, 2, 20, 0)} + assert C.format_game_date(self.WEEKDAY, log, "10/3", game) == \ + f"{C.weekday_for(self.WEEKDAY, log, game)} Oct 3" + + @pytest.mark.parametrize("game", [None, {}, {"start_time_utc": "garbage"}]) + def test_no_usable_start_draws_no_weekday(self, log, game): + assert C.format_game_date(self.WEEKDAY, log, "10/2", game) == "Oct 2" + + def test_the_scorebug_twin_formats_the_same(self, log): + """Switch mode (SportsCoreSharedMixin) shares the formatter body.""" + from src.common.sports_shared import SportsCoreSharedMixin + + class Host(SportsCoreSharedMixin): + config = {"scroll_card": {"date_format": "weekday", + "switch_date_format": "inherit"}} + logger = log + + def _get_timezone(self): + return ZoneInfo("America/New_York") + + game, want = self._as_printed("2026-11-01T00:30:00+00:00", "America/New_York") + assert Host()._format_game_date(game["game_date"], game) == want + assert C.format_game_date(Host.config, log, game["game_date"], game) == want + + class TestFontSizing: def test_snaps_to_the_faces_pixel_grid(self): assert C.crisp_size("4x6-font.ttf", 6) == 7 # 7px grid diff --git a/test/test_sports_twins.py b/test/test_sports_twins.py index 4202468d..32f5cc47 100644 --- a/test/test_sports_twins.py +++ b/test/test_sports_twins.py @@ -561,21 +561,24 @@ class TestPinnedDivergence: assert C.recent_score_color(on, LOG, game, (9, 9, 9)) == (9, 9, 9) def test_weekday_zone_source(self): - # DIVERGENCE, user-visible: the scorebug asks the plugin's + # DIVERGENCE, not drawn: the scorebug asks the plugin's # _get_timezone() (plugin setting -> global setting -> system zone); # the card reads only config["timezone"] and falls back to UTC. The # scoreboards' schemas default that key to "", and the scroll display - # hands the renderer the plugin config, so a board that sets only the - # global zone gets UTC weekdays in scroll mode: an evening kickoff in - # New York is labelled with the next day. + # hands the renderer the plugin config, so the bare weekday helpers + # still disagree for an evening kickoff in New York. game = {"start_time_utc": "2026-09-20T00:30:00+00:00"} # Sat 20:30 EDT host = _Host({}, tz=ZoneInfo("America/New_York")) assert host._weekday_for(game) == "Sat" assert C.weekday_for({}, LOG, game) == "Sun" + # DECIDED: what a card draws is the printed date's own weekday, so + # the scroll card no longer labels that kickoff with the next day + # ("Sun Sep 19" before). Both formatters place the extractor's "M/D" + # against the start time instead of re-deriving the day in a zone. cfg = {"scroll_card": {"date_format": "weekday", "switch_date_format": "inherit"}} host = _Host(cfg, tz=ZoneInfo("America/New_York")) assert host._format_game_date("9/19", game) == "Sat Sep 19" - assert C.format_game_date(cfg, LOG, "9/19", game) == "Sun Sep 19" + assert C.format_game_date(cfg, LOG, "9/19", game) == "Sat Sep 19" def test_weekday_out_of_range_start(self): # DIVERGENCE: the mixin catches OverflowError from astimezone() and diff --git a/test/test_store_symlinked_plugin.py b/test/test_store_symlinked_plugin.py new file mode 100644 index 00000000..6cbbb5d6 --- /dev/null +++ b/test/test_store_symlinked_plugin.py @@ -0,0 +1,90 @@ +"""Removing a dev plugin linked into the plugins directory removes the link. + +``scripts/dev/dev_plugin_setup.sh`` symlinks a checkout into the plugins +directory. ``PluginStoreManager._safe_remove_directory`` -- behind uninstall, +and behind discarding the set-aside copy after an install or update -- handed +the link to ``shutil.rmtree``, which refuses a symlink. Its fallback then +walked through the link and chmodded every directory and file of the linked +checkout to 0700, and the sudo stage refused a path outside the plugins +directory. So the uninstall failed, the link stayed, and the developer's +checkout lost its group/other permissions and gained execute bits. + +Skipped where this process cannot create a symlink (Windows without the +privilege). +""" + +import json +import os +from unittest.mock import MagicMock + +import pytest + +from src.plugin_system.store_manager import PluginStoreManager + +PLUGIN_ID = "linked-demo" + + +def _symlink_or_skip(target, link): + try: + os.symlink(target, link, target_is_directory=True) + except (OSError, NotImplementedError) as e: + pytest.skip(f"cannot create a symlink here: {e}") + + +@pytest.fixture +def linked(tmp_path): + checkout = tmp_path / "dev-plugins" / PLUGIN_ID + checkout.mkdir(parents=True) + (checkout / "manifest.json").write_text( + json.dumps({"id": PLUGIN_ID, "name": "Linked", "class_name": "P", + "display_modes": ["linked"], "version": "1.0.0"}), + encoding="utf-8") + (checkout / "manager.py").write_text("X = 1\n", encoding="utf-8") + plugins_dir = tmp_path / "plugins" + plugins_dir.mkdir() + link = plugins_dir / PLUGIN_ID + _symlink_or_skip(checkout, link) + store = PluginStoreManager(plugins_dir=str(plugins_dir)) + store.logger = MagicMock() + return store, link, checkout + + +def test_removing_a_linked_plugin_removes_only_the_link(linked): + store, link, checkout = linked + + assert store._safe_remove_directory(link) is True + + assert not os.path.lexists(link) + assert (checkout / "manager.py").read_text(encoding="utf-8") == "X = 1\n" + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX permission bits") +def test_removing_a_linked_plugin_leaves_the_checkouts_permissions(linked): + store, link, checkout = linked + os.chmod(checkout, 0o755) + os.chmod(checkout / "manager.py", 0o644) + + store._safe_remove_directory(link) + + assert checkout.stat().st_mode & 0o777 == 0o755 + assert (checkout / "manager.py").stat().st_mode & 0o777 == 0o644 + + +def test_uninstalling_a_linked_plugin_removes_the_link(linked): + store, link, checkout = linked + + assert store.uninstall_plugin(PLUGIN_ID) is True + + assert not os.path.lexists(link) + assert (checkout / "manifest.json").exists() + + +def test_a_dangling_link_is_removed_too(linked): + store, link, checkout = linked + for child in checkout.iterdir(): + child.unlink() + checkout.rmdir() + + assert store._safe_remove_directory(link) is True + + assert not os.path.lexists(link) diff --git a/web_interface/blueprints/api_v3/plugin_store.py b/web_interface/blueprints/api_v3/plugin_store.py index 23aca5a9..e776918d 100644 --- a/web_interface/blueprints/api_v3/plugin_store.py +++ b/web_interface/blueprints/api_v3/plugin_store.py @@ -42,6 +42,29 @@ def _store_incompatibility(plugin: dict) -> Optional[str]: return reason if isinstance(reason, str) and reason else None +def _installed_plugin_id(plugin_id: str) -> str: + """The id the plugin installed for store entry ``plugin_id`` declares. + + A registry entry can install under another id: ``weather`` installs a + directory whose manifest says ``ledmatrix-weather``, and that is the id + the plugin list, the config section and /plugins/toggle know it by. The + install is found the way the store's update and uninstall find it (the + entry's id, ``aliases`` and ``plugin_path`` name); ``plugin_id`` itself + when its manifest can't be read. + """ + try: + plugin_dir = api_v3.plugin_store_manager._find_plugin_path(plugin_id) + manifest_path = (resolve_under(plugin_dir, 'manifest.json') + if isinstance(plugin_dir, Path) else None) + if manifest_path is None or not manifest_path.is_file(): + return plugin_id + with open(manifest_path, 'r', encoding='utf-8') as f: + manifest_id = json.load(f).get('id') + except Exception: # noqa: BLE001 - only names the install for the client + return plugin_id + return manifest_id if isinstance(manifest_id, str) and safe_path_component(manifest_id) else plugin_id + + def _listed_plugin_dir(base: Path, name: str) -> Optional[Path]: """The entry of ``base`` called ``name``, or None. @@ -487,8 +510,10 @@ def install_plugin(): ) branch_msg = f" (branch: {branch})" if branch else "" + # plugin_id: the id to enable it by (see _installed_plugin_id). return {'success': True, 'message': f'Plugin {plugin_id} installed successfully{branch_msg}', + 'plugin_id': _installed_plugin_id(plugin_id), **_store_restart_fields('install', _plugin_enabled_in_config(plugin_id))} else: error_msg = f'Failed to install plugin {plugin_id}' @@ -546,7 +571,8 @@ def install_plugin(): branch_msg = f" (branch: {branch})" if branch else "" return success_response( message=f'Plugin installed successfully{branch_msg}', - extra=_store_restart_fields('install', _plugin_enabled_in_config(plugin_id))) + extra={'plugin_id': _installed_plugin_id(plugin_id), + **_store_restart_fields('install', _plugin_enabled_in_config(plugin_id))}) else: error_msg = f'Failed to install plugin {plugin_id}' if branch: diff --git a/web_interface/blueprints/api_v3/plugins.py b/web_interface/blueprints/api_v3/plugins.py index 8c6bd2a3..ec319e3e 100644 --- a/web_interface/blueprints/api_v3/plugins.py +++ b/web_interface/blueprints/api_v3/plugins.py @@ -145,6 +145,14 @@ def get_installed_plugins(): vegas_participation, vegas_participation_source = _vegas_participation( plugin_id, plugin_config, plugin_info) + # The modes the manifest declares, from the catalog as /display/modes + # and on-demand/start read them. The on-demand modal offers these; + # without them it offered only the plugin id, which the display + # turns into the first mode. Strings only: a manifest is hand-edited. + declared_modes = api_v3.plugin_catalog.get_plugin_display_modes(plugin_id) + display_modes = ([m for m in declared_modes if isinstance(m, str)] + if isinstance(declared_modes, list) else []) + return { 'id': plugin_id, 'name': plugin_info.get('name', plugin_id), @@ -158,6 +166,7 @@ def get_installed_plugins(): # The tab nav uses this as the element's Font Awesome class # (app-shell.js / app-early.js); only a string can be one. 'icon': plugin_info.get('icon') if isinstance(plugin_info.get('icon'), str) else None, + 'display_modes': display_modes, 'enabled': enabled, 'verified': verified, # loaded, state, error_info, loaded_version, loaded_at: the diff --git a/web_interface/static/v3/js/plugins/install_manager.js b/web_interface/static/v3/js/plugins/install_manager.js index 8ed36398..7641ef8c 100644 --- a/web_interface/static/v3/js/plugins/install_manager.js +++ b/web_interface/static/v3/js/plugins/install_manager.js @@ -48,12 +48,15 @@ const PluginInstallManager = { * @returns {Promise} Update results, one per plugin sent */ async updateAll(onProgress, options = {}) { - // Prefer PluginStateManager if populated, fall back to window.installedPlugins - // (plugins_manager.js populates window.installedPlugins independently) - const stateManagerPlugins = window.PluginStateManager && window.PluginStateManager.installedPlugins; - const listed = (stateManagerPlugins && stateManagerPlugins.length > 0) - ? stateManagerPlugins - : (window.installedPlugins || []); + // window.installedPlugins is the live list: plugins_manager.js + // republishes it after every install, uninstall and refresh. + // PluginStateManager's copy is written only by the refresh at the end + // of a run, so preferring it sent a second run the first run's + // plugins -- an uninstalled one failed, a new one was skipped. It is + // the fallback for a page without the plugin manager. + const listed = Array.isArray(window.installedPlugins) + ? window.installedPlugins + : ((window.PluginStateManager && window.PluginStateManager.installedPlugins) || []); // Snapshot: the list can be replaced while this loop is awaiting. const plugins = this.updatablePlugins(listed); @@ -102,10 +105,18 @@ const PluginInstallManager = { } // Reload plugin list once at the end. A failed refresh must not - // lose the results: they carry the restart flags. - if (window.PluginStateManager) { + // lose the results: they carry the restart flags. The plugin + // manager's load, forced past its caches, also redraws the installed + // grid and its Updates badge; PluginStateManager's only replaced + // window.installedPlugins, so the cards kept offering "Update to vX" + // for what had just been updated. + const pluginManager = window.pluginManager; + const refresh = (pluginManager && typeof pluginManager.loadInstalledPlugins === 'function') + ? () => pluginManager.loadInstalledPlugins(true) + : (window.PluginStateManager ? () => window.PluginStateManager.loadInstalledPlugins() : null); + if (refresh) { try { - await window.PluginStateManager.loadInstalledPlugins(); + await refresh(); } catch (error) { console.warn('Could not refresh the installed plugin list after updating:', error); } diff --git a/web_interface/static/v3/plugins_manager.js b/web_interface/static/v3/plugins_manager.js index 1664dabd..d9ae0300 100644 --- a/web_interface/static/v3/plugins_manager.js +++ b/web_interface/static/v3/plugins_manager.js @@ -34,8 +34,8 @@ * * Layout: a few handlers defined up front, outside any IIFE, because the * cards and other scripts call them through window (configurePlugin, - * togglePlugin, the GitHub token helpers, handleGitHubPluginInstall, - * checkGitHubAuthStatus); then the plugin-manager IIFE (private state: + * togglePlugin, the GitHub token helpers, checkGitHubAuthStatus); then the + * plugin-manager IIFE (private state: * installedPlugins, the store cache, the on-demand poller); then the * Starlark IIFE. * @@ -386,103 +386,6 @@ window.toggleGithubTokenContent = function(e) { } }; -// Simple standalone handler for GitHub plugin installation -// Defined early and globally to ensure it's always available -debugLog('[DEFINE] Defining handleGitHubPluginInstall function...'); -window.handleGitHubPluginInstall = function() { - debugLog('[handleGitHubPluginInstall] Function called!'); - - const urlInput = document.getElementById('github-plugin-url'); - const statusDiv = document.getElementById('github-plugin-status'); - const branchInput = document.getElementById('plugin-branch-input'); - const installBtn = document.getElementById('install-plugin-from-url'); - - if (!urlInput) { - console.error('[handleGitHubPluginInstall] URL input not found'); - alert('Error: Could not find URL input field'); - return; - } - - const repoUrl = urlInput.value.trim(); - debugLog('[handleGitHubPluginInstall] Repo URL:', repoUrl); - - if (!repoUrl) { - if (statusDiv) { - statusDiv.innerHTML = 'Please enter a GitHub URL'; - } - return; - } - - if (!isGithubUrl(repoUrl)) { - if (statusDiv) { - statusDiv.innerHTML = 'Please enter a valid GitHub URL'; - } - return; - } - - // Disable button and show loading - if (installBtn) { - installBtn.disabled = true; - installBtn.innerHTML = 'Installing...'; - } - if (statusDiv) { - statusDiv.innerHTML = 'Installing plugin...'; - } - - const branch = branchInput?.value?.trim() || null; - const requestBody = { repo_url: repoUrl }; - if (branch) { - requestBody.branch = branch; - } - - debugLog('[handleGitHubPluginInstall] Sending request:', requestBody); - - fetch('/api/v3/plugins/install-from-url', { - method: 'POST', - headers: { - 'Content-Type': 'application/json' - }, - body: JSON.stringify(requestBody) - }) - .then(response => { - debugLog('[handleGitHubPluginInstall] Response status:', response.status); - return response.json(); - }) - .then(data => { - debugLog('[handleGitHubPluginInstall] Response data:', data); - if (data.status === 'success') { - if (statusDiv) { - statusDiv.innerHTML = `Successfully installed: ${window.LEDEscape.html(data.plugin_id)}`; - } - urlInput.value = ''; - - showNotification(`Plugin ${data.plugin_id} installed successfully`, 'success'); - window.noteRestartRequired(data); - - setTimeout(() => window.pluginManager.loadInstalledPlugins(true).catch(() => {}), 1000); - } else { - if (statusDiv) { - statusDiv.innerHTML = `${window.LEDEscape.html(data.message || 'Installation failed')}`; - } - showNotification(data.message || 'Installation failed', 'error'); - } - }) - .catch(error => { - console.error('[handleGitHubPluginInstall] Error:', error); - if (statusDiv) { - statusDiv.innerHTML = `Error: ${window.LEDEscape.html(error.message)}`; - } - showNotification('Error installing plugin: ' + error.message, 'error'); - }) - .finally(() => { - if (installBtn) { - installBtn.disabled = false; - installBtn.innerHTML = 'Install'; - } - }); -}; -debugLog('[DEFINE] handleGitHubPluginInstall defined and ready'); - // GitHub Authentication Status - Define early so it's available in IIFE // Shows warning banner only when token is missing or invalid // The token itself is never exposed to the frontend for security @@ -2083,6 +1986,13 @@ window.uninstallPlugin = function(pluginId) { }); } +// How many times the store's Install polls a queued install, a second apart. +// The server allows the plugin's dependency install 300 s on its own +// (install_requirements_file in src/plugin_system/store_install.py), after a +// download that fetches the plugin a file at a time; the 60 the poller +// defaults to reported installs that then succeeded as timed out. +const INSTALL_POLL_MAX_ATTEMPTS = 600; + function pollOperationStatus(operationId, pluginId, pluginName, options = {}) { const maxAttempts = options.maxAttempts || 60; const attempt = options.attempt || 0; @@ -2114,9 +2024,10 @@ function pollOperationStatus(operationId, pluginId, pluginName, options = {}) { if (status === 'completed') { // The operation's result says whether the display picks - // the change up by itself or needs a restart. + // the change up by itself or needs a restart, and for an + // install which id the plugin was installed as. window.noteRestartRequired(operation.result); - onComplete(); + onComplete(operation.result); } else if (status === 'failed') { onFailed(operation.error || operation.message); } else { @@ -2270,10 +2181,18 @@ function showStoreLoading(show) { // ── Plugin Store: Client-Side Filter/Sort/Pagination ──────────────────────── function isStorePluginInstalled(pluginIdOrPlugin) { + return Boolean(findInstalledStorePlugin(pluginIdOrPlugin)); +} + +// The installed-list entry for a store plugin, or undefined. A registry entry +// can be installed under another id -- `weather` is listed as the +// `ledmatrix-weather` its manifest declares -- so its own id is tried first, +// then its plugin_path name, then its aliases. +function findInstalledStorePlugin(pluginIdOrPlugin) { const installed = window.installedPlugins || installedPlugins || []; // Accept either a plain ID string or a store plugin object (which may have plugin_path) if (typeof pluginIdOrPlugin === 'string') { - return installed.some(p => p.id === pluginIdOrPlugin); + return installed.find(p => p.id === pluginIdOrPlugin); } const storeId = pluginIdOrPlugin.id; // Derive the actual installed directory name from plugin_path (e.g. "plugins/ledmatrix-weather" → "ledmatrix-weather") @@ -2281,8 +2200,9 @@ function isStorePluginInstalled(pluginIdOrPlugin) { const pathDerivedId = pluginPath ? pluginPath.split('/').pop() : null; // Newer registries also list the other ids outright (the manifest id). const aliases = Array.isArray(pluginIdOrPlugin.aliases) ? pluginIdOrPlugin.aliases : []; - return installed.some(p => p.id === storeId || (pathDerivedId && p.id === pathDerivedId) - || aliases.includes(p.id)); + return installed.find(p => p.id === storeId) + || (pathDerivedId ? installed.find(p => p.id === pathDerivedId) : undefined) + || installed.find(p => aliases.includes(p.id)); } // ── Plugin Store: search / filter / sort ──────────────────────────────── @@ -2392,8 +2312,43 @@ function getStoreFilter() { return _storeFilter; } +// The category filter offers the categories the store's plugins have, as the +// Starlark section does. The template ships only "All Categories": a fixed +// list offered 7 of the registry's ~20 categories, so most plugins could not +// be filtered to, and "Financial" missed the plugin filed under "finance". +// One option per category whatever its case (the filter ignores case), and +// rebuilt only when the set changes, or for a select freshly swapped in. +function syncStoreCategoryOptions() { + const select = document.getElementById('plugin-category'); + if (!select) return; + const ctl = getStoreFilter(); + const selected = String((ctl ? ctl.state.filterCategory : select.value) || ''); + const byKey = new Map(); + (pluginStoreCache || []).forEach(plugin => { + const category = plugin && typeof plugin.category === 'string' ? plugin.category : ''; + if (category.trim() && !byKey.has(category.toLowerCase())) { + byKey.set(category.toLowerCase(), category); + } + }); + // The current choice stays selectable even if no plugin has it any more. + if (selected && !byKey.has(selected.toLowerCase())) byKey.set(selected.toLowerCase(), selected); + const categories = [...byKey.values()].sort((a, b) => a.localeCompare(b, undefined, { sensitivity: 'base' })); + const key = categories.join('\n'); + if (select._storeCategories === key) return; + select._storeCategories = key; + select.innerHTML = ''; + categories.forEach(category => { + const option = document.createElement('option'); + option.value = category; + option.textContent = category.charAt(0).toUpperCase() + category.slice(1); + select.appendChild(option); + }); + select.value = selected; +} + function applyStoreFiltersAndSort(skipPageReset) { if (!pluginStoreCache) return; + syncStoreCategoryOptions(); const ctl = getStoreFilter(); if (ctl) { ctl.apply(skipPageReset); @@ -2510,11 +2465,45 @@ window.installPlugin = function(pluginId, branch = null) { requestBody.branch = branch; } - function enableAfterInstall() { + const storeEntry = (pluginStoreCache || []).find(p => p && p.id === pluginId) || { id: pluginId }; + // Decided before the install changes the list, by the same match that + // labelled the button Install or Reinstall. A reinstall keeps the plugin + // as the user had it: enabling it here switched a deliberately disabled + // plugin back on. + const isReinstall = isStorePluginInstalled(storeEntry); + + // The id the plugin was installed as, which can differ from the store's: + // `weather` installs as the `ledmatrix-weather` its manifest declares, + // and that is the id /plugins/toggle knows. The install answer names it + // (plugin_id); from one that doesn't, the installed entry the store + // entry matches, as for the Installed badge. + function installedPluginId(result) { + if (result && typeof result.plugin_id === 'string' && result.plugin_id) { + return result.plugin_id; + } + const match = findInstalledStorePlugin(storeEntry); + return match ? match.id : pluginId; + } + + function afterInstall(result) { + // Reload first, so the new card exists (and, without plugin_id in the + // answer, so the installed id can be found), then redraw the store's + // badges from that list. + loadInstalledPlugins(true).catch(() => {}).then(() => { + applyStoreFiltersAndSort(true); + if (isReinstall) { + showNotification(`${pluginId} reinstalled`, 'success'); + return; + } + enableAfterInstall(installedPluginId(result)); + }); + } + + function enableAfterInstall(installedId) { // Enable immediately so install -> enable is one step; only nudge // for a restart once enablement actually succeeded (persistent // toast; duration 0 = stays until dismissed). - Promise.resolve(window.togglePlugin(pluginId, true)).then(toggleResult => { + Promise.resolve(window.togglePlugin(installedId, true)).then(toggleResult => { if (toggleResult && toggleResult.status === 'success') { showNotification( `${pluginId} installed and enabled — restart the display to show it`, @@ -2532,9 +2521,6 @@ window.installPlugin = function(pluginId, branch = null) { ); } }); - // Refresh installed plugins list, then re-render store to update badges - loadInstalledPlugins().catch(() => {}); - setTimeout(() => applyStoreFiltersAndSort(true), 500); } fetch('/api/v3/plugins/install', { @@ -2555,14 +2541,25 @@ window.installPlugin = function(pluginId, branch = null) { // live: "installation queued" followed immediately by a failed // enable). Wait for the operation to actually finish first. pollOperationStatus(data.data.operation_id, pluginId, pluginId, { - onComplete: enableAfterInstall, + onComplete: afterInstall, onFailed: (errorMsg) => showNotification(errorMsg || `Failed to install ${pluginId}`, 'error'), - onTimeout: () => showNotification(`Install operation timed out for ${pluginId}`, 'error') + maxAttempts: INSTALL_POLL_MAX_ATTEMPTS, + // Out of patience is not a failure: the server may still be + // installing. Show the list as it is now and say so; nothing + // is enabled without the operation's answer. + onTimeout: () => { + showNotification( + `${pluginId} is still installing — it will appear in the installed list when it finishes`, + 'warning' + ); + loadInstalledPlugins(true).catch(() => {}) + .then(() => applyStoreFiltersAndSort(true)); + } }); } else { // No operation queue configured - install already completed synchronously. window.noteRestartRequired(data); - enableAfterInstall(); + afterInstall(data); } }) .catch(error => { diff --git a/web_interface/templates/v3/partials/plugins.html b/web_interface/templates/v3/partials/plugins.html index cec9a472..72c718c7 100644 --- a/web_interface/templates/v3/partials/plugins.html +++ b/web_interface/templates/v3/partials/plugins.html @@ -250,13 +250,7 @@ @@ -467,8 +461,9 @@ -