diff --git a/plugin-repos/starlark-apps/manager.py b/plugin-repos/starlark-apps/manager.py index 3da8c24d5..fad2dc7d4 100644 --- a/plugin-repos/starlark-apps/manager.py +++ b/plugin-repos/starlark-apps/manager.py @@ -681,12 +681,18 @@ def update(self) -> None: if app.is_enabled() and app.should_render(current_time): self._render_app(app, force=False) - def display(self, force_clear: bool = False) -> None: + def display(self, force_clear: bool = False) -> bool: """ Display current Starlark app. This method is called during the display rotation. Displays frames from the currently active app. + + Returns False when there is no app to show -- which is the state of + every install without Pixlet, and of a fresh one before any app is + added. The display controller only skips a mode on a boolean False + (it checks isinstance(result, bool)), so returning None held a black + panel for the full display_duration instead of rotating on. """ try: if force_clear: @@ -699,20 +705,24 @@ def display(self, force_clear: bool = False) -> None: if not self.current_app: # No apps available self.logger.debug("No Starlark apps to display") - return + return False # Render app if needed if not self.current_app.frames: success = self._render_app(self.current_app, force=True) if not success: self.logger.error(f"Failed to render app: {self.current_app.app_id}") - return + return False - # Display current frame - self._display_frame() + # Display current frame. The result is propagated: a failed frame + # update is not a displayed frame, and returning True regardless + # told the controller the mode had rendered, so it held the dead + # frame for the whole display_duration instead of rotating on. + return self._display_frame() except Exception as e: self.logger.error(f"Error displaying Starlark app: {e}") + return False def _select_next_app(self) -> None: """Select the next enabled app for display.""" @@ -835,10 +845,13 @@ def _load_frames_from_cache(self, app: StarlarkApp) -> bool: self.logger.error(f"Error loading frames for {app.app_id}: {e}") return False - def _display_frame(self) -> None: - """Display the current frame of the current app.""" + def _display_frame(self) -> bool: + """Display the current frame of the current app. + + :returns: whether a frame actually reached the display manager. + """ if not self.current_app or not self.current_app.frames: - return + return False try: current_time = time.time() @@ -856,8 +869,11 @@ def _display_frame(self) -> None: ) self.current_app.last_frame_time = current_time + return True + except Exception as e: self.logger.error(f"Error displaying frame: {e}") + return False def install_app(self, app_id: str, star_file_path: str, metadata: Optional[Dict[str, Any]] = None, assets_dir: Optional[str] = None) -> bool: """ diff --git a/scripts/run_plugin_tests.py b/scripts/run_plugin_tests.py index 61e597100..fcaa84824 100755 --- a/scripts/run_plugin_tests.py +++ b/scripts/run_plugin_tests.py @@ -6,6 +6,9 @@ Supports both unittest and pytest. """ +import os +import re +import subprocess # nosec B404 - list-form argv only, no shell # nosemgrep import sys import argparse from pathlib import Path @@ -68,6 +71,79 @@ def _find_tests_in_dir(directory: Path) -> list: return sorted(set(test_files)) +def _is_script_style(path) -> bool: + """True when a test file is a standalone script, not a pytest module. + + Most plugin tests are written as `def main()` plus an `if __name__ == + "__main__"` guard and signal through an exit code. pytest collects zero + items from those, so handing them to pytest printed "no tests ran" and this + runner reported success over work it had not done -- 151 of 248 files on a + fully populated rig. + """ + try: + src = Path(path).read_text(encoding="utf-8", errors="replace") + except OSError: + return False + has_pytest_items = re.search(r"^\s*(def test_|class Test|async def test_)", src, re.M) + has_main_guard = "__main__" in src and "__name__" in src + return bool(has_main_guard and not has_pytest_items) + + +def run_script_tests(test_files: list, verbose: bool = False) -> int: + """Run standalone test scripts, honouring the 0 pass / 2 skip / 1 fail + convention that ledmatrix-plugins' own runner established. + + Scripts opt into skipping by printing "SKIP: " and exiting 2 -- + a script that needs a tty or an LED matrix is not a regression. + """ + env = dict(os.environ) + # Prepend rather than setdefault. An inherited PYTHONPATH -- a developer's + # shell, a tox run, another checkout -- otherwise wins outright, and the + # subprocess imports a different copy of the core than the one under test. + # That is exactly the failure ledmatrix-plugins#467 describes, and it is + # invisible: the tests pass or fail against a tree nobody meant to test. + inherited = env.get("PYTHONPATH") + env["PYTHONPATH"] = (f"{PROJECT_ROOT}{os.pathsep}{inherited}" + if inherited else str(PROJECT_ROOT)) + env["LEDMATRIX_CORE"] = str(PROJECT_ROOT) + + passed = skipped = failed = 0 + failures = [] + for path in test_files: + try: + # Fixed interpreter (sys.executable) plus a test path this script + # discovered by globbing the repo; argument list, no shell, so + # nothing is word-split or expanded. Same suppression pair the + # rest of the repo uses for this shape (see permission_utils.py). + proc = subprocess.run( # noqa: S603 # nosec B603 - no shell invoked (list-form argv) # nosemgrep + [sys.executable, str(path)], # nosemgrep + cwd=str(Path(path).parent), + capture_output=True, text=True, env=env, + stdin=subprocess.DEVNULL, timeout=300, + ) + rc = proc.returncode + tail = " | ".join((proc.stdout or proc.stderr or "").strip().splitlines()[-2:])[:200] + except subprocess.TimeoutExpired: + rc, tail = 1, "timed out after 300s" + if rc == 0: + passed += 1 + label = "pass" + elif rc == 2: + skipped += 1 + label = "SKIP" + else: + failed += 1 + label = "FAIL" + failures.append(f"{Path(path).name}: exit {rc} | {tail}") + if verbose or rc != 0: + print(f" [{label}] {Path(path).name}" + (f" -- {tail}" if rc != 0 else "")) + + print(f"\n{passed} passed, {skipped} skipped, {failed} failed (scripts)") + for f in failures: + print(f" - {f}", file=sys.stderr) + return 1 if failed else 0 + + def run_unittest_tests(test_files: list, verbose: bool = False) -> int: """ Run tests using unittest. @@ -186,11 +262,16 @@ def main(): print("No test files found in plugins directory") return 0 - print(f"Found {len(test_files)} test file(s)") + scripts = [f for f in test_files if _is_script_style(f)] + modules = [f for f in test_files if f not in scripts] + + print(f"Found {len(test_files)} test file(s)" + + (f" -- {len(modules)} collectable, {len(scripts)} standalone script(s)" + if scripts else "")) for test_file in test_files: print(f" - {test_file}") print() - + # Determine runner runner = args.runner if runner == 'auto': @@ -199,12 +280,21 @@ def main(): runner = 'pytest' except ImportError: runner = 'unittest' - - # Run tests - if runner == 'pytest': - return run_pytest_tests(test_files, args.verbose, args.coverage) - else: - return run_unittest_tests(test_files, args.verbose) + + # Standalone scripts cannot be collected by pytest or unittest -- run them + # as the scripts they are. Doing this rather than silently collecting zero + # items is the whole point: this runner used to report success having + # executed nothing. + rc = 0 + if scripts: + rc |= run_script_tests(scripts, args.verbose) + + if modules: + if runner == 'pytest': + rc |= run_pytest_tests(modules, args.verbose, args.coverage) + else: + rc |= run_unittest_tests(modules, args.verbose) + return rc if __name__ == '__main__': diff --git a/src/display_controller.py b/src/display_controller.py index bb6506988..0e56580d6 100644 --- a/src/display_controller.py +++ b/src/display_controller.py @@ -198,6 +198,10 @@ def _follower_gated_update(): # the main run loop reconciles (loads/unloads) on its own thread so # mutating available_modes never races with rendering. self._pending_plugin_reconcile = False + # Monotonic stamp of the last mailbox disk read; see + # _poll_on_demand_requests. None means "never polled", so the first + # call always goes through. + self._last_on_demand_poll: Optional[float] = None self.on_demand_active = False self.on_demand_mode: Optional[str] = None self.on_demand_modes: List[str] = [] # All modes for the on-demand plugin @@ -1267,12 +1271,34 @@ def _set_on_demand_error(self, message: str) -> None: self.on_demand_schedule_override = False self._publish_on_demand_state() + #: Shortest gap between mailbox disk reads. This is called after every + #: frame -- about 125 times a second on a scrolling mode -- and the read + #: below is deliberately uncached, so without a floor it was 125 disk reads + #: per second to find nothing. An on-demand request comes from a person + #: clicking in the web UI, so a quarter second of latency is not + #: perceptible, and it cuts the read rate by 30x. + ON_DEMAND_POLL_INTERVAL = 0.25 + def _poll_on_demand_requests(self) -> None: """Poll cache for new on-demand requests from external controllers.""" + now = time.monotonic() + if (self._last_on_demand_poll is not None + and now - self._last_on_demand_poll < self.ON_DEMAND_POLL_INTERVAL): + return + self._last_on_demand_poll = now + try: # Use a long max_age (1 hour) to ensure requests aren't expired before processing - # The request_id check prevents duplicate processing - request = self.cache_manager.get('display_on_demand_request', max_age=3600) + # The request_id check prevents duplicate processing. + # + # memory_ttl=0 is required, not optional: this key is a mailbox the + # web process writes and this process reads. get() defaults the + # in-memory TTL to max_age, so without it the first request read was + # pinned in memory for the full hour and every later poll returned + # that stale copy -- meaning no second on-demand request was honoured + # for an hour, while the API still reported success. + request = self.cache_manager.get('display_on_demand_request', + max_age=3600, memory_ttl=0) except (OSError, RuntimeError, ValueError, TypeError) as err: logger.error("Failed to read on-demand request: %s", err, exc_info=True) return @@ -1318,6 +1344,36 @@ def _poll_on_demand_requests(self) -> None: # Mark as processed BEFORE processing (to prevent duplicate processing) self.cache_manager.set('display_on_demand_processed_id', request_id, ttl=3600) self.on_demand_request_id = request_id + # Consume the mailbox entry. Leaving it on disk meant a restart replayed + # the previous request: the fresh controller read it, activated it and + # cached it, so the request the caller had just made was ignored and the + # panel silently showed the earlier plugin. processed_id still guards + # against double-processing if this delete fails. + try: + # Compare before deleting. The web process can post a newer request + # between the read above and this delete; an unconditional delete + # threw that one away and it was never processed -- the user's + # second click did nothing. Re-reading uncached and only deleting + # our own request_id means a newer request is left in the mailbox + # for the next poll instead. + # + # This narrows the window rather than closing it: a request landing + # between this re-read and the delete is still lost. Closing it + # properly needs an atomic claim (a rename, or a compare-and-delete + # primitive) that the cache layer does not currently offer, so the + # honest fix is a smaller window plus this note, not a bigger lock. + current = self.cache_manager.get('display_on_demand_request', + max_age=3600, memory_ttl=0) + if not current or current.get('request_id') == request_id: + self.cache_manager.delete('display_on_demand_request') + else: + logger.debug("Newer on-demand request %s arrived while processing " + "%s; leaving it in the mailbox", + current.get('request_id'), request_id) + except (OSError, AttributeError, KeyError) as err: + # Best-effort: processed_id still guards against reprocessing if the + # mailbox cannot be cleared. + logger.debug("Could not clear the on-demand request mailbox: %s", err) if action == 'start': logger.info("Processing on-demand start request for plugin: %s", request.get('plugin_id')) diff --git a/src/display_manager.py b/src/display_manager.py index e578ad7cb..076e92203 100644 --- a/src/display_manager.py +++ b/src/display_manager.py @@ -1453,12 +1453,30 @@ def _write_snapshot_if_due(self) -> None: if parent_dir and str(parent_dir) != '/tmp': # nosec B108 - guard to skip /tmp for permission ops ensure_directory_permissions(parent_dir, get_assets_dir_mode()) self._snapshot_dir_prepared = True - # Write atomically: temp then replace - tmp_path = f"{self._snapshot_path}.tmp" - self.image.save(tmp_path, format='PNG') + # Write atomically: temp then replace. The temp name must be + # unique, not ".tmp": /tmp is world-writable and sticky, + # and this file is written by whichever user the display service + # runs as while tests and tooling run as someone else. A leftover + # fixed-name temp owned by another user is then unopenable even by + # root (fs.protected_regular refuses O_CREAT on a foreign file in a + # sticky dir), which froze the preview and the health check's + # liveness proxy until somebody deleted it by hand. Same pattern as + # the hardware-status write above. + _fd, tmp_path = tempfile.mkstemp( + dir=str(snapshot_path_obj.parent), + prefix=f".{snapshot_path_obj.name}.", suffix=".tmp") try: + with os.fdopen(_fd, "wb") as _f: + self.image.save(_f, format='PNG') + os.chmod(tmp_path, 0o644) os.replace(tmp_path, self._snapshot_path) except Exception: + # Never leave the temp behind -- that is what made the failure + # permanent rather than transient. + try: + os.unlink(tmp_path) + except OSError: + pass # Fallback to direct save if replace not supported self.image.save(self._snapshot_path, format='PNG') # Set proper file permissions after saving diff --git a/src/font_manager.py b/src/font_manager.py index 8be283beb..936408083 100644 --- a/src/font_manager.py +++ b/src/font_manager.py @@ -96,7 +96,8 @@ def __init__(self, config: Dict[str, Any]): self.common_fonts = { "press_start": "assets/fonts/PressStart2P-Regular.ttf", "four_by_six": "assets/fonts/4x6-font.ttf", - "five_by_seven": "assets/fonts/5x7.bdf" + "five_by_seven": "assets/fonts/5x7.bdf", + "tom_thumb": "assets/fonts/tom-thumb.bdf" # Note: cozette_bdf removed - font file not available # To re-enable: download cozette.bdf from https://github.com/the-moonwitch/Cozette # and add: "cozette_bdf": "assets/fonts/cozette.bdf" diff --git a/src/plugin_system/testing/harness.py b/src/plugin_system/testing/harness.py index 51c688a29..267774c2d 100644 --- a/src/plugin_system/testing/harness.py +++ b/src/plugin_system/testing/harness.py @@ -14,6 +14,8 @@ import contextlib import http.client import inspect +import time +from datetime import timedelta import socket import ssl import urllib.error @@ -25,7 +27,7 @@ from src.logging_config import get_logger from .bounds_display_manager import BoundsCheckingDisplayManager -from .loading import load_config_defaults, load_manifest +from .loading import load_config_defaults, load_manifest, merge_config from .sizes import DEFAULT_TEST_SIZES, safe_mode_filename, size_label logger = get_logger("[Plugin Harness]") @@ -116,14 +118,23 @@ def list_modes(plugin_instance: Any, manifest: Dict[str, Any], plugin_id: str) - def _instantiate(plugin_id: str, manifest: Dict[str, Any], plugin_dir: Path, config: Dict[str, Any], mock_data: Dict[str, Any], - display_manager: Any) -> Any: - """Load and construct a plugin instance with mocked managers.""" + display_manager: Any, cache_manager: Any = None) -> Any: + """Load and construct a plugin instance with mocked managers. + + Pass ``cache_manager`` to share one cache across the renders of a plugin. + Building a fresh one per (size, mode) made every render a cold start, so a + plugin that fetches per game or per player re-fetched everything N times -- + baseball-scoreboard took 840s for nine renders where ~72s was the arithmetic + -- and the cache-hit path, which is what a running rig executes almost + always, was never exercised. + """ from src.plugin_system.plugin_loader import PluginLoader from src.plugin_system.testing import MockCacheManager, MockPluginManager - cache_manager = MockCacheManager() - for key, value in (mock_data or {}).items(): - cache_manager.set(key, value) + if cache_manager is None: + cache_manager = MockCacheManager() + for key, value in (mock_data or {}).items(): + cache_manager.set(key, value) loader = PluginLoader() plugin_instance, _module = loader.load_plugin( @@ -160,6 +171,107 @@ def _render_mode(plugin_instance: Any, mode: str) -> Any: return plugin_instance.display(force_clear=False) +# How many extra frames to drive before believing a mode really draws nothing. +# A scroll starts with its content off-panel, so frame 1 is legitimately blank; +# measured across the fleet, content appears by frame 2-4 (f1-scoreboard), +# frame 4 (ledmatrix-elections) and frame 38 at 64px (ledmatrix-leaderboard). +EMPTY_RECHECK_FRAMES = 48 +# Seconds to advance the clock between those frames. Scroll position is usually +# driven by elapsed time, which a frozen clock never provides. +EMPTY_RECHECK_STEP = 0.05 + + +def _has_content(image) -> bool: + """True when any pixel is lit above the threshold.""" + if image is None: + return False + return image.convert("L").point( + lambda p: 255 if p > _LIT_THRESHOLD else 0).getbbox() is not None + + +def _render_mode_again(plugin_instance: Any, mode: str) -> Any: + """Draw one more frame WITHOUT force_clear. + + _render_mode passes force_clear=True, which for a scrolling plugin means + "reset the scroll to the start" -- so repeating it would redraw frame 1 for + ever. The re-check needs the plugin to advance. + """ + sig = inspect.signature(plugin_instance.display) + if "display_mode" in sig.parameters: + return plugin_instance.display(force_clear=False, display_mode=mode) + return plugin_instance.display(force_clear=False) + + +def _settle_empty_frame(inst, mode, dm, result, freezer) -> None: + """Give an apparently-empty mode a few frames to draw before believing it. + + One frame is not evidence: a scroll's first frame is its blank scroll-in + buffer. Without this, every scrolling plugin was warned about -- 60 of 76 + warnings on a 44-plugin rig were false, which is the rate at which people + stop reading a warning. + """ + if result.error is not None or result.display_returned is False: + return + if _has_content(result.image): + return + # The frozen clock is shared by every render in the matrix, so any time this + # probe borrows has to be given back -- otherwise a mode that scrolls in + # leaves the clock advanced and every later mode renders at the wrong + # instant, drifting its golden. Seen as 5 spurious f1_upcoming drifts. + resume_at = None + if freezer is not None: + try: + resume_at = freezer() + except (AttributeError, TypeError, ValueError): + # Not a freezegun factory, or a version whose factory is not + # callable. Only used to restore the clock, never load-bearing. + resume_at = None + try: + _settle_loop(inst, mode, dm, result, freezer) + finally: + if resume_at is not None: + try: + freezer.move_to(resume_at) + except (AttributeError, TypeError, ValueError): + pass + + +def _settle_loop(inst, mode, dm, result, freezer) -> None: + tick = getattr(freezer, "tick", None) if freezer is not None else None + for _ in range(EMPTY_RECHECK_FRAMES): + if tick is not None: + # timedelta rather than a bare float: freezegun has accepted a + # number only since 1.x, and a stale pin would raise here. + try: + tick(timedelta(seconds=EMPTY_RECHECK_STEP)) + except (AttributeError, TypeError, ValueError): + # Pacing is best-effort; a freezegun that will not take a + # timedelta just means this probe runs without advancing time. + pass + else: + # No frozen clock, so the real one has to do the advancing. Without + # this the 48 frames run in microseconds, elapsed time stays ~0, and + # a scroll driven by elapsed time never moves -- which is exactly + # the plugin this check is trying not to slander. + time.sleep(EMPTY_RECHECK_STEP) + try: + result.display_returned = _render_mode_again(inst, mode) + except Exception as e: # noqa: BLE001 + # Deliberately broad: this calls a plugin's display(), which can + # raise anything. Recorded rather than swallowed -- a mode that + # renders one good frame and then crashes on the next is broken, + # and returning silently here reported it as passing. The frame + # already captured stays on the result so the failure is still + # inspectable. + result.error = repr(e) + return + image = dm.get_image() + if _has_content(image): + result.image = image + result.overflow = dm.check_overflow() + return + + def _freeze(freeze_time: Optional[str]): """Context manager that freezes wall-clock time when freeze_time is given, so time-dependent plugins (clocks, countdowns) render deterministic goldens.""" @@ -193,7 +305,9 @@ def render_plugin_matrix( manifest = load_manifest(plugin_dir) # Start from config_schema.json defaults so the plugin behaves like a real # install; explicit caller config still wins over a schema default. - config = {"enabled": True, **load_config_defaults(plugin_dir), **(config or {})} + config = merge_config( + merge_config({"enabled": True}, load_config_defaults(plugin_dir)), + config or {}) sizes = sizes or DEFAULT_TEST_SIZES results: List[RenderResult] = [] @@ -202,25 +316,35 @@ def render_plugin_matrix( # rendering a smaller one, instead of being clipped into a false pass. extent = (max(w for w, _ in sizes), max(h for _, h in sizes)) - with _freeze(freeze_time): + # One cache for the whole matrix: see _instantiate. The display manager + # stays per-render (the bounds checking depends on that); only fetched data + # is shared. + from src.plugin_system.testing import MockCacheManager + cache_manager = MockCacheManager() + for key, value in (mock_data or {}).items(): + cache_manager.set(key, value) + + with _freeze(freeze_time) as freezer: for width, height in sizes: results.extend(_render_size( plugin_id, manifest, plugin_dir, config, mock_data or {}, - width, height, run_update, extent, + width, height, run_update, extent, cache_manager, freezer, )) return results def _render_size(plugin_id, manifest, plugin_dir, config, mock_data, - width, height, run_update, extent) -> List[RenderResult]: + width, height, run_update, extent, + cache_manager=None, freezer=None) -> List[RenderResult]: """Render every mode at one size. A fresh instance per mode avoids state leaks.""" results: List[RenderResult] = [] # Discover modes once per size (instance build can depend on config). try: probe_dm = BoundsCheckingDisplayManager(width=width, height=height, overflow_extent=extent) - probe = _instantiate(plugin_id, manifest, plugin_dir, config, mock_data, probe_dm) + probe = _instantiate(plugin_id, manifest, plugin_dir, config, mock_data, probe_dm, + cache_manager) modes = list_modes(probe, manifest, plugin_id) except Exception as e: # noqa: BLE001 — surface any load failure as a result return [RenderResult(plugin_id, width, height, "", error=repr(e))] @@ -229,7 +353,8 @@ def _render_size(plugin_id, manifest, plugin_dir, config, mock_data, result = RenderResult(plugin_id, width, height, mode) dm = BoundsCheckingDisplayManager(width=width, height=height, overflow_extent=extent) try: - inst = _instantiate(plugin_id, manifest, plugin_dir, config, mock_data, dm) + inst = _instantiate(plugin_id, manifest, plugin_dir, config, mock_data, dm, + cache_manager) if run_update: try: inst.update() @@ -248,6 +373,9 @@ def _render_size(plugin_id, manifest, plugin_dir, config, mock_data, result.display_returned = _render_mode(inst, mode) result.image = dm.get_image() result.overflow = dm.check_overflow() + # A blank first frame is not proof of a blank mode; see + # _settle_empty_frame. + _settle_empty_frame(inst, mode, dm, result, freezer) except Exception as e: # noqa: BLE001 — a display crash is a real failure result.error = repr(e) results.append(result) diff --git a/src/plugin_system/testing/loading.py b/src/plugin_system/testing/loading.py index 061e95a09..326b3e73a 100644 --- a/src/plugin_system/testing/loading.py +++ b/src/plugin_system/testing/loading.py @@ -33,6 +33,45 @@ def load_manifest(plugin_dir: Union[str, Path]) -> Dict[str, Any]: return json.load(f) +def _defaults_from_properties(properties: Dict[str, Any]) -> Dict[str, Any]: + """Defaults for one `properties` block, recursing into nested objects. + + An object property carries its defaults on its children, not on itself, so + reading only the top level dropped everything nested. That is most of the + fleet: config organised by league, or under customization/display_options, + lost 2,386 defaults across 37 of 44 plugins -- soccer-scoreboard alone lost + 539 of 565 -- and the harness rendered them with a config no install would + ever have. + """ + defaults: Dict[str, Any] = {} + for key, prop in (properties or {}).items(): + if not isinstance(prop, dict): + continue + if prop.get('type') == 'object' and isinstance(prop.get('properties'), dict): + nested = _defaults_from_properties(prop['properties']) + if nested: + defaults[key] = nested + elif 'default' in prop: + defaults[key] = prop['default'] + return defaults + + +def merge_config(base: Dict[str, Any], override: Dict[str, Any]) -> Dict[str, Any]: + """Deep-merge override onto base, without dropping sibling defaults. + + A shallow merge would let `-c '{"nhl": {"enabled": true}}'` replace the whole + nhl subtree and silently discard every other nhl default -- the same class of + bug this function exists to fix. + """ + merged = dict(base) + for key, value in (override or {}).items(): + if isinstance(value, dict) and isinstance(merged.get(key), dict): + merged[key] = merge_config(merged[key], value) + else: + merged[key] = value + return merged + + def load_config_defaults(plugin_dir: Union[str, Path]) -> Dict[str, Any]: """Extract default values from a plugin's config_schema.json (empty if none).""" schema_path = Path(plugin_dir) / 'config_schema.json' @@ -40,11 +79,7 @@ def load_config_defaults(plugin_dir: Union[str, Path]) -> Dict[str, Any]: return {} with open(schema_path, 'r') as f: schema = json.load(f) - defaults: Dict[str, Any] = {} - for key, prop in schema.get('properties', {}).items(): - if isinstance(prop, dict) and 'default' in prop: - defaults[key] = prop['default'] - return defaults + return _defaults_from_properties(schema.get('properties', {})) def load_harness_spec(plugin_dir: Union[str, Path]) -> Dict[str, Any]: diff --git a/test/test_harness_empty_claimed.py b/test/test_harness_empty_claimed.py index 2791516ab..97e6b074b 100644 --- a/test/test_harness_empty_claimed.py +++ b/test/test_harness_empty_claimed.py @@ -101,3 +101,61 @@ def test_a_result_with_no_image_is_left_alone(self): r = _result(None, returned=None) check_empty_claimed([r], strict=True) assert r.empty_claimed is None + + +class TestSettleRecordsLaterFailures: + """A mode that renders one good frame and then crashes is broken. + + _settle_loop re-renders a mode that came back blank, to give a scroll or an + animation time to put something on the panel. Swallowing an exception from + those later frames meant the harness reported a passing result for a mode + that crashes as soon as it is asked for a second frame -- exactly the kind + of defect the harness exists to catch. + """ + + class Boom: + """Renders once, then raises.""" + + def __init__(self): + self.calls = 0 + + def display(self, force_clear=False): + self.calls += 1 + raise RuntimeError("second frame exploded") + + def _settle(self, inst, dm, result): + from src.plugin_system.testing import harness + harness._settle_loop(inst, "mode", dm, result, None) + + def test_the_exception_is_recorded_on_the_result(self, monkeypatch): + from src.plugin_system.testing import harness + # Keep the probe short; this test is about the error, not the pacing. + monkeypatch.setattr(harness, "EMPTY_RECHECK_FRAMES", 1) + monkeypatch.setattr(harness, "EMPTY_RECHECK_STEP", 0) + + result = _result(_blank()) + assert result.error is None + self._settle(self.Boom(), _FakeDM(), result) + + assert result.error is not None, "a crash on a later frame was swallowed" + assert "second frame exploded" in result.error + + def test_the_already_captured_frame_is_kept(self, monkeypatch): + from src.plugin_system.testing import harness + monkeypatch.setattr(harness, "EMPTY_RECHECK_FRAMES", 1) + monkeypatch.setattr(harness, "EMPTY_RECHECK_STEP", 0) + + image = _blank() + result = _result(image) + self._settle(self.Boom(), _FakeDM(), result) + assert result.image is image, "the good frame was discarded along with the error" + + +class _FakeDM: + """Minimal display-manager double for _settle_loop.""" + + def get_image(self): + return _blank() + + def check_overflow(self): + return None diff --git a/test/test_on_demand_mailbox.py b/test/test_on_demand_mailbox.py new file mode 100644 index 000000000..dceca2d4b --- /dev/null +++ b/test/test_on_demand_mailbox.py @@ -0,0 +1,106 @@ +"""The on-demand request mailbox: how often it is read, and how it is consumed. + +The mailbox is a cache key the web process writes and the display process +reads. Two properties matter and neither is obvious from the call site: + + * it is polled after every rendered frame, so an uncached read here is a + disk read at frame rate; + * consuming it must not throw away a request that arrived while the previous + one was being processed. +""" + +from unittest.mock import MagicMock + +import pytest + + +class TestPollingIsBounded: + """_poll_on_demand_requests runs ~125x/second on a scrolling mode. + + The read is deliberately uncached (memory_ttl=0) because a cached one + pinned the first request for an hour. That makes the call a real disk read, + so it needs a floor -- without one it was ~125 reads per second to find + nothing at all. + """ + + def test_first_call_always_reads(self, test_display_controller): + c = test_display_controller + c.cache_manager.get = MagicMock(return_value=None) + c._poll_on_demand_requests() + assert c.cache_manager.get.call_count == 1 + + def test_immediate_second_call_does_not_read(self, test_display_controller): + c = test_display_controller + c.cache_manager.get = MagicMock(return_value=None) + c._poll_on_demand_requests() + for _ in range(50): + c._poll_on_demand_requests() + assert c.cache_manager.get.call_count == 1, "polling was not bounded" + + def test_reads_again_once_the_interval_has_passed(self, test_display_controller, monkeypatch): + c = test_display_controller + c.cache_manager.get = MagicMock(return_value=None) + clock = {"t": 1000.0} + monkeypatch.setattr("src.display_controller.time.monotonic", lambda: clock["t"]) + + c._poll_on_demand_requests() + clock["t"] += c.ON_DEMAND_POLL_INTERVAL / 2 + c._poll_on_demand_requests() + assert c.cache_manager.get.call_count == 1, "read before the interval elapsed" + + clock["t"] += c.ON_DEMAND_POLL_INTERVAL + c._poll_on_demand_requests() + assert c.cache_manager.get.call_count == 2 + + def test_the_interval_is_short_enough_to_feel_instant(self, test_display_controller): + # A person clicking in the web UI must not notice the floor. + assert test_display_controller.ON_DEMAND_POLL_INTERVAL <= 0.5 + + +class TestMailboxIsConsumedByIdentity: + """Deleting whatever is in the mailbox loses a request that raced in.""" + + def _arrange(self, controller, first, later): + """Mailbox returns `first`, then `later` on the pre-delete re-read.""" + controller.on_demand_active = False + controller.on_demand_request_id = None + controller._last_on_demand_poll = None + reads = iter([first, later]) + + def fake_get(key, *a, **kw): + if key == 'display_on_demand_request': + return next(reads, later) + return None # processed-id lookup + + controller.cache_manager.get = MagicMock(side_effect=fake_get) + controller.cache_manager.set = MagicMock() + controller.cache_manager.delete = MagicMock() + controller._activate_on_demand = MagicMock() + + REQ_A = {'request_id': 'A', 'action': 'start', 'plugin_id': 'p', 'mode': 'm'} + REQ_B = {'request_id': 'B', 'action': 'start', 'plugin_id': 'p', 'mode': 'm'} + + def test_own_request_is_deleted(self, test_display_controller): + c = test_display_controller + self._arrange(c, self.REQ_A, self.REQ_A) + c._poll_on_demand_requests() + c.cache_manager.delete.assert_called_once_with('display_on_demand_request') + + def test_a_newer_request_is_left_for_the_next_poll(self, test_display_controller): + c = test_display_controller + self._arrange(c, self.REQ_A, self.REQ_B) + c._poll_on_demand_requests() + assert c.cache_manager.delete.call_count == 0, \ + "request B was deleted without ever being processed" + + def test_an_already_empty_mailbox_is_still_cleared(self, test_display_controller): + c = test_display_controller + self._arrange(c, self.REQ_A, None) + c._poll_on_demand_requests() + c.cache_manager.delete.assert_called_once_with('display_on_demand_request') + + def test_the_request_is_still_processed(self, test_display_controller): + c = test_display_controller + self._arrange(c, self.REQ_A, self.REQ_B) + c._poll_on_demand_requests() + c._activate_on_demand.assert_called_once() diff --git a/test/test_starlark_display_contract.py b/test/test_starlark_display_contract.py new file mode 100644 index 000000000..1804d7bf2 --- /dev/null +++ b/test/test_starlark_display_contract.py @@ -0,0 +1,82 @@ +"""starlark-apps: display() must report what actually reached the panel. + +The display controller skips a mode only on a boolean False. Returning True +after the frame update failed told it the mode had rendered, so it held a dead +frame for the whole display_duration instead of rotating on -- the same class +of defect as a display() that returns None. +""" + +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +PLUGIN_DIR = Path(__file__).resolve().parent.parent / "plugin-repos" / "starlark-apps" + + +@pytest.fixture(scope="module") +def manager_module(): + if not PLUGIN_DIR.exists(): + pytest.skip("starlark-apps plugin is not checked out") + sys.path.insert(0, str(PLUGIN_DIR)) + try: + import importlib + spec = importlib.util.spec_from_file_location( + "starlark_manager_under_test", PLUGIN_DIR / "manager.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + except Exception as e: # noqa: BLE001 - optional deps (pixlet, fcntl) may be absent + pytest.skip(f"starlark-apps manager is not importable here: {e}") + finally: + sys.path.remove(str(PLUGIN_DIR)) + + +def _plugin(manager_module): + """A manager with __init__ bypassed -- only display paths are under test.""" + cls = manager_module.StarlarkAppsPlugin + inst = cls.__new__(cls) + inst.logger = MagicMock() + inst.display_manager = MagicMock() + inst.current_app = None + return inst + + +class _App: + def __init__(self, frames): + self.frames = frames + self.current_frame_index = 0 + self.last_frame_time = 0.0 + self.app_id = "app" + + +class TestDisplayFramePropagates: + def test_a_failed_update_returns_false(self, manager_module): + p = _plugin(manager_module) + p.current_app = _App([("frame", 100)]) + p.display_manager.update_display.side_effect = RuntimeError("panel gone") + + assert p._display_frame() is False + + def test_a_good_update_returns_true(self, manager_module): + p = _plugin(manager_module) + p.current_app = _App([("frame", 100)]) + + assert p._display_frame() is True + + def test_no_frames_returns_false(self, manager_module): + p = _plugin(manager_module) + p.current_app = _App([]) + + assert p._display_frame() is False + + def test_display_reports_the_frame_failure(self, manager_module): + p = _plugin(manager_module) + p.current_app = _App([("frame", 100)]) + p.display_manager.update_display.side_effect = RuntimeError("panel gone") + + result = p.display() + + assert result is False, "display() claimed success over a failed frame update" + assert isinstance(result, bool), "the controller only skips on a real bool" diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 35dfd885f..845c68848 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -78,8 +78,11 @@ def _scrub_git_remote_url(url: str) -> str: return url # Will be initialized when blueprint is registered -config_manager = None -plugin_manager = None +# NOTE: the managers live on the blueprint object (app.py sets +# api_v3.config_manager / api_v3.plugin_manager). Deliberately not +# mirrored as module globals: a bare `config_manager` used to resolve to +# a None that was never assigned, which silently disabled the /health +# checks and made /display/current fall back to a hardcoded 128x64. plugin_store_manager = None saved_repositories_manager = None cache_manager = None @@ -1598,11 +1601,16 @@ def get_health(): } # Check web interface service + # Stamp the start time before measuring against it -- reading it with a + # fallback of time.time() and only assigning afterwards made the very + # first call subtract two separate clock reads, reporting a small + # negative uptime. + if not hasattr(get_health, '_start_time'): + get_health._start_time = time.time() health_status['services']['web_interface'] = { 'status': 'running', - 'uptime_seconds': time.time() - (getattr(get_health, '_start_time', time.time())) + 'uptime_seconds': time.time() - get_health._start_time } - get_health._start_time = getattr(get_health, '_start_time', time.time()) # Check display service display_service_status = _get_display_service_status() @@ -1613,8 +1621,8 @@ def get_health(): # Check config file accessibility try: - if config_manager: - test_config = config_manager.load_config() + if api_v3.config_manager: + test_config = api_v3.config_manager.load_config() health_status['checks']['config_file'] = { 'status': 'accessible', 'readable': True @@ -1633,9 +1641,9 @@ def get_health(): # Check plugin system try: - if plugin_manager: + if api_v3.plugin_manager: # Try to discover plugins (lightweight check) - plugin_count = len(plugin_manager.get_available_plugins()) if hasattr(plugin_manager, 'get_available_plugins') else 0 + plugin_count = len(api_v3.plugin_manager.get_available_plugins()) if hasattr(api_v3.plugin_manager, 'get_available_plugins') else 0 health_status['checks']['plugin_system'] = { 'status': 'operational', 'plugin_count': plugin_count @@ -2468,8 +2476,8 @@ def get_display_current(): # Get display dimensions from config try: - if config_manager: - main_config = config_manager.load_config() + if api_v3.config_manager: + main_config = api_v3.config_manager.load_config() hardware_config = main_config.get('display', {}).get('hardware', {}) cols = hardware_config.get('cols', 64) chain_length = hardware_config.get('chain_length', 2)