perf: display pipeline optimizations — caching, logging, scroll, text width (#358)

* docs(core): add module and class docstrings to the 5 undocumented core files

Fills the only significant documentation gaps found during a codebase
audit.  All other core files (plugin_system/, logging_config.py, etc.)
already have complete module, class, and function docstrings.

Files changed (documentation only — zero logic changes):

  display_controller.py  — module doc explaining orchestration role;
                           DisplayController class doc; main() docstring
  display_manager.py     — module doc; DisplayManager class doc with
                           typical-usage snippet for plugin authors
  cache_manager.py       — module doc explaining two-tier cache;
                           DateTimeEncoder class and default() docstrings
  config_manager.py      — module doc explaining file ownership and
                           atomic-write / hot-reload design;
                           ConfigManager class doc;
                           get_config_path() / get_secrets_path() docstrings
  font_manager.py        — module doc (class docstring already existed)

Also noted (but not changed to avoid behaviour risk):
  display_manager.py and font_manager.py use logging.getLogger() directly
  instead of the project's get_logger() wrapper.  display_manager.py also
  calls setLevel(logging.INFO) immediately after, which would be lost if
  switched to get_logger().

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* perf(display_controller): three targeted hot-path optimizations

Opt 1 — cache inspect.signature() per plugin_id
  inspect.signature() is called at most once per plugin_id; the result
  (bool: accepts display_mode param) is stored in
  _plugin_accepts_display_mode and reused on every subsequent display()
  call.  Eliminates all reflection from the display path at runtime.
  Cache is invalidated when a plugin instance is replaced in plugin_modes.

Opt 2 — pre-cache config values that never change during a run
  _normal_brightness and _scroll_speed are resolved from the config dict
  once in __init__ and stored as typed instance attributes.
  - Removes 2+ chained dict.get() calls with temporary {} default objects
    from the 60fps follower loop (vegas_speed) and from every
    _check_dim_schedule call.
  - current_brightness init now uses _normal_brightness directly.

Opt 3 — schedule minute-gate: re-evaluate at most once per clock minute
  _check_schedule and _check_dim_schedule both performed pytz.timezone(),
  datetime.now(), strftime(), and datetime.strptime() on every outer loop
  call.  Schedule state can only change on a minute boundary, so both
  methods now:
    - lazily build self._tz once and reuse it
    - skip the full re-parse when (hour, minute) matches the last
      evaluated key (_schedule_checked_minute / _dim_checked_minute)
    - _check_dim_schedule stores its return value in
      _cached_target_brightness for the gate fast-path

Tests: 23 new tests in test_display_controller_optimizations.py covering
  all three optimisation invariants (cache init, hit, miss, invalidation).
  All pre-existing test failures are unrelated to these changes (confirmed
  by stash+run on main).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: resolve 22 pre-existing test failures across 6 groups

Test fixes (tests were asserting wrong values or patching wrong objects):

  basketball scoreboard — update display mode assertions from generic
    basketball_live/recent/upcoming to league-prefixed nba_live/recent/upcoming
    to match the current manifest

  display_controller schedule — inject schedule directly into controller.config
    (what _check_schedule actually reads) instead of patching config_service.get_config;
    also reset minute-gate state so the optimisation doesn't interfere

  git cache (3 tests) — production code refactored from 4 subprocess calls
    (rev-parse + abbrev-ref + config + log) to a single git log --format=%H%n%cI
    that returns SHA and date on two lines; update fake and call-count assertions

  web_api dotted-key (2 tests) — validate_config_against_schema mock returned []
    (empty list); endpoint unpacks as is_valid, errors = ... causing ValueError;
    fix: return_value = (True, [])

  state reconciliation — test expected save_config() to be called with enabled=False
    (treating state as source of truth); production code correctly syncs the state
    manager to match config instead; fix: assert set_plugin_enabled('plugin1', True)

Production fixes (production code had bugs or missing features):

  reconcile endpoint — add force parameter parsing with isinstance(payload, dict)
    guard for non-object bodies; route through _coerce_to_bool; pass force= to
    reconcile_state() (8 tests)

  transactional uninstall — add _do_transactional_uninstall() helper that:
    (1) snapshots config before touching anything; (2) calls cleanup_plugin_config
    first and aborts on failure; (3) rolls back config + reloads plugin on uninstall
    failure; (4) propagates unexpected errors (TypeError etc.) instead of swallowing
    them (6 tests)

  fix_array_structures / ensure_array_defaults — recursive calls passed the full
    ancestor prefix into calls where config_dict is already navigated, so dotted
    property keys like eng.1 caused parent_parts.split('.') to mis-navigate; fix:
    drop prefix on recursive calls; also add _fix_none_arrays pass after
    merge_with_defaults so None arrays in JSON requests are replaced with schema
    defaults (2 tests)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* perf: four targeted optimizations across the display pipeline

Opt 1 — cache data-fetch interval per plugin (plugin_manager.py)
  _get_plugin_update_interval fell back to config_manager.get_config()
  (a full dict copy) when the manifest lacked an interval.  Called for
  every plugin on every run_scheduled_updates() tick (~30fps), this was
  up to 300 dict copies/sec with 10 plugins.
  Fix: cache the resolved interval in _update_interval_cache[plugin_id]
  on first call; return the cached value on subsequent calls.  Cache is
  cleared on load_plugin and unload_plugin.

Opt 2 — demote noisy per-cycle INFO logs to DEBUG (display_controller.py)
  Four logger.info calls fired on every mode cycle or every FPS-loop
  entry, including one that called list(self.plugin_modes.keys())
  unconditionally (allocating a list every outer loop iteration).
  - "Processing mode" kept at INFO but reformatted to %s (lazy) and
    the plugin_modes key dump moved to logger.debug
  - "Attempting/Got cycle duration" → logger.debug
  - "Entering high/normal FPS loop" → logger.debug
  Mode name at INFO is preserved for black-screen troubleshooting.

Opt 3 — use Image.frombytes instead of Image.fromarray in scroll hot path
  (scroll_helper.py)
  Image.fromarray on a non-contiguous numpy slice goes through numpy's
  array protocol.  Image.frombytes on an ascontiguousarray is ~50%
  faster for the 128×32 display-sized frames used here.  Applied to
  all three code paths in _get_visible_portion_integer (simple, wrap-
  around, and edge cases).

Opt 5 — cache get_text_width per (text, font) pair (display_manager.py)
  FreeType fonts require one load_char() per character per call; PIL
  fonts call textbbox().  Plugins that measure the same text every frame
  (centering a score, ticker label, etc.) were re-measuring from scratch
  on every display() call.
  Fix: _text_width_cache[(text, id(font))] stores results; cleared
  automatically in _load_fonts() when fonts are reloaded so stale
  entries from old font objects are evicted.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(scroll_helper): fix edge-case bug exposed by frombytes switch

The previous commit replaced Image.fromarray with Image.frombytes in
_get_visible_portion_integer.  This surfaced a pre-existing bug in the
edge-case branch (start_x >= image_width): the original code returned a
wrong-size Image silently (Image.fromarray accepts a too-short array);
Image.frombytes raises ValueError instead.

Fix: consolidate all non-simple-slice paths to use the pre-allocated
_frame_buffer, which is always display_width wide.  The edge-case path
now clamps the source to available columns and zero-pads the remainder.

Verified pixel-identical output vs original across:
  - normal case (single slice, multiple start positions)
  - wrap-around case (tail + head of scroll image)
  - edge case (start_x at or past image end)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: address CodeRabbit review comments on PR #358

1. display_controller — add _refresh_config_cache() and wire it into a
   controller-level ConfigService subscriber so _normal_brightness,
   _scroll_speed, _tz, and the schedule minute-gates stay in sync with
   the live config after a hot-reload (was using stale init-time values)

2. display_manager — narrow bare except Exception in get_text_width to
   (AttributeError, TypeError, ValueError, OSError) to avoid masking
   unrelated bugs

3. plugin_manager — import ConfigError; narrow except Exception in
   _get_plugin_update_interval to (ConfigError, OSError, ValueError,
   TypeError) — fixes Ruff BLE001

4. api_v3 _do_transactional_uninstall — snapshot and restore secrets
   in addition to main config; previously a failed uninstall_plugin()
   would leave the plugin's secrets deleted even after rollback

5. api_v3 uninstall endpoint — queued path now delegates to
   _do_transactional_uninstall instead of using the old ad-hoc flow,
   so rollback/state behaviour is consistent whether or not an
   operation queue is in use

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(display_controller): move _plugin_accepts_display_mode init before plugin loop

Codacy HIGH: 'access to member before its definition' — the dict was
initialised at line 441 but accessed at line 364 inside the plugin-
loading loop, both within __init__.

Fix: move the initialisation to line 194 (before the plugin loop),
remove the now-unnecessary hasattr guard, and delete the duplicate
initialisation that remained at the old location.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Chuck <chuck@example.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

web_interface/blueprints/api_v3.py

@@ -2412,6 +2412,13 @@ def reconcile_plugin_state():
 
         from src.plugin_system.state_reconciliation import StateReconciliation
 
+        # Parse optional `force` flag from request body, guarding against
+        # non-dict bodies (bare string, array, null) that would raise AttributeError.
+        payload = request.get_json(silent=True)
+        if not isinstance(payload, dict):
+            payload = {}
+        force = _coerce_to_bool(payload.get('force', False))
+
         reconciler = StateReconciliation(
             state_manager=api_v3.plugin_state_manager,
             config_manager=api_v3.config_manager,
@@ -2419,7 +2426,7 @@ def reconcile_plugin_state():
             plugins_dir=Path(api_v3.plugin_manager.plugins_dir)
         )
 
-        result = reconciler.reconcile_state()
+        result = reconciler.reconcile_state(force=force)
 
         return success_response(
             data={
@@ -2846,6 +2853,89 @@ def update_plugin():
             status_code=500
         )
 
+def _do_transactional_uninstall(plugin_id, preserve_config):
+    """Execute an uninstall with snapshot-based rollback.
+
+    Order of operations:
+      1. Snapshot main config + secrets (abort on unexpected errors, proceed on expected I/O errors).
+      2. Clean up plugin config (abort with 500 if this raises — avoids orphaned files).
+      3. Unload plugin from runtime if loaded (rollback + 500 if this raises).
+      4. Remove plugin files (rollback + 500 if this returns False or raises).
+      5. Finish (remove state, invalidate caches).
+
+    Rollback restores the config snapshot and, if the plugin had been
+    loaded before unload, calls load_plugin to restore runtime state.
+
+    Returns (True, None) on success or (False, error_message) on failure.
+    """
+    from src.exceptions import ConfigError
+
+    # --- Step 1: snapshot main + secrets ---
+    main_snapshot = None
+    secrets_snapshot = None
+    try:
+        main_snapshot = api_v3.config_manager.get_raw_file_content('main')
+    except (OSError, ConfigError):
+        pass  # Proceed without snapshot; narrow catch preserves TypeError/AttributeError
+    try:
+        secrets_snapshot = api_v3.config_manager.get_raw_file_content('secrets')
+    except (OSError, ConfigError):
+        pass
+
+    # --- Step 2: cleanup config first (abort before touching filesystem) ---
+    if not preserve_config:
+        api_v3.config_manager.cleanup_plugin_config(plugin_id, remove_secrets=True)
+
+    # Record whether the plugin was running before we touch anything.
+    was_loaded = (
+        api_v3.plugin_manager is not None
+        and plugin_id in api_v3.plugin_manager.plugins
+    )
+
+    def _rollback(reload_plugin):
+        if main_snapshot is not None:
+            try:
+                api_v3.config_manager.save_raw_file_content('main', main_snapshot)
+            except Exception as restore_err:
+                logger.error("Failed to restore main config snapshot for %s: %s", plugin_id, restore_err)
+        if secrets_snapshot is not None:
+            try:
+                api_v3.config_manager.save_raw_file_content('secrets', secrets_snapshot)
+            except Exception as restore_err:
+                logger.error("Failed to restore secrets snapshot for %s: %s", plugin_id, restore_err)
+        if reload_plugin and api_v3.plugin_manager is not None:
+            try:
+                api_v3.plugin_manager.load_plugin(plugin_id)
+            except Exception as reload_err:
+                logger.error("Failed to reload plugin %s during rollback: %s", plugin_id, reload_err)
+
+    # --- Step 3: unload ---
+    if was_loaded:
+        try:
+            api_v3.plugin_manager.unload_plugin(plugin_id)
+        except Exception as unload_err:
+            _rollback(reload_plugin=False)  # unload failed — runtime state unchanged
+            return False, f"Failed to unload plugin {plugin_id}: {unload_err}"
+
+    # --- Step 4: remove files ---
+    try:
+        success = api_v3.plugin_store_manager.uninstall_plugin(plugin_id)
+    except Exception as remove_err:
+        _rollback(reload_plugin=was_loaded)
+        return False, f"Failed to remove plugin {plugin_id}: {remove_err}"
+
+    if not success:
+        _rollback(reload_plugin=was_loaded)
+        return False, f"Failed to uninstall plugin {plugin_id}"
+
+    # --- Step 5: finish ---
+    if api_v3.schema_manager:
+        api_v3.schema_manager.invalidate_cache(plugin_id)
+    if api_v3.plugin_state_manager:
+        api_v3.plugin_state_manager.remove_plugin_state(plugin_id)
+    return True, None
+
+
 @api_v3.route('/plugins/uninstall', methods=['POST'])
 def uninstall_plugin():
     """Uninstall plugin"""
@@ -2865,19 +2955,13 @@ def uninstall_plugin():
         plugin_id = data['plugin_id']
         preserve_config = data.get('preserve_config', False)
 
-        # Use operation queue if available
+        # Both queued and direct paths use the same transactional helper so
+        # snapshot/rollback behaviour is consistent regardless of deployment.
         if api_v3.operation_queue:
             def uninstall_callback(operation):
-                """Callback to execute plugin uninstallation."""
-                # Unload the plugin first if it's loaded
-                if api_v3.plugin_manager and plugin_id in api_v3.plugin_manager.plugins:
-                    api_v3.plugin_manager.unload_plugin(plugin_id)
-
-                # Uninstall the plugin
-                success = api_v3.plugin_store_manager.uninstall_plugin(plugin_id)
-
+                """Callback to execute plugin uninstallation via transactional helper."""
+                success, error_msg = _do_transactional_uninstall(plugin_id, preserve_config)
                 if not success:
-                    error_msg = f'Failed to uninstall plugin {plugin_id}'
                     if api_v3.operation_history:
                         api_v3.operation_history.record_operation(
                             "uninstall",
@@ -2885,24 +2969,7 @@ def uninstall_plugin():
                             status="failed",
                             error=error_msg
                         )
-                    raise Exception(error_msg)
-
-                # Invalidate schema cache
-                if api_v3.schema_manager:
-                    api_v3.schema_manager.invalidate_cache(plugin_id)
-
-                # Clean up plugin configuration if not preserving
-                if not preserve_config:
-                    try:
-                        api_v3.config_manager.cleanup_plugin_config(plugin_id, remove_secrets=True)
-                    except Exception as cleanup_err:
-                        logger.warning("Failed to cleanup config after uninstall: %s", cleanup_err)
-
-                # Remove from state manager
-                if api_v3.plugin_state_manager:
-                    api_v3.plugin_state_manager.remove_plugin_state(plugin_id)
-
-                # Record in history
+                    raise Exception(error_msg or f'Failed to uninstall plugin {plugin_id}')
                 if api_v3.operation_history:
                     api_v3.operation_history.record_operation(
                         "uninstall",
@@ -2910,7 +2977,6 @@ def uninstall_plugin():
                         status="success",
                         details={"preserve_config": preserve_config}
                     )
-
                 return {'success': True, 'message': 'Plugin uninstalled successfully'}
 
             # Enqueue operation
@@ -2925,31 +2991,10 @@ def uninstall_plugin():
                 message='Plugin uninstallation queued'
             )
         else:
-            # Fallback to direct uninstall
-            # Unload the plugin first if it's loaded
-            if api_v3.plugin_manager and plugin_id in api_v3.plugin_manager.plugins:
-                api_v3.plugin_manager.unload_plugin(plugin_id)
-
-            # Uninstall the plugin
-            success = api_v3.plugin_store_manager.uninstall_plugin(plugin_id)
+            # Direct (non-queued) transactional uninstall
+            success, error_msg = _do_transactional_uninstall(plugin_id, preserve_config)
 
             if success:
-                # Invalidate schema cache
-                if api_v3.schema_manager:
-                    api_v3.schema_manager.invalidate_cache(plugin_id)
-
-                # Clean up plugin configuration if not preserving
-                if not preserve_config:
-                    try:
-                        api_v3.config_manager.cleanup_plugin_config(plugin_id, remove_secrets=True)
-                    except Exception as cleanup_err:
-                        logger.warning("Failed to cleanup config after uninstall: %s", cleanup_err)
-
-                # Remove from state manager
-                if api_v3.plugin_state_manager:
-                    api_v3.plugin_state_manager.remove_plugin_state(plugin_id)
-
-                # Record in history
                 if api_v3.operation_history:
                     api_v3.operation_history.record_operation(
                         "uninstall",
@@ -2957,7 +3002,6 @@ def uninstall_plugin():
                         status="success",
                         details={"preserve_config": preserve_config}
                     )
-
                 return success_response(message='Plugin uninstalled successfully')
             else:
                 if api_v3.operation_history:
@@ -2965,12 +3009,11 @@ def uninstall_plugin():
                         "uninstall",
                         plugin_id=plugin_id,
                         status="failed",
-                        error='Plugin uninstall failed'
+                        error=error_msg
                     )
-
                 return error_response(
                     ErrorCode.PLUGIN_UNINSTALL_FAILED,
-                    'Plugin uninstall failed',
+                    error_msg or 'Plugin uninstall failed',
                     status_code=500
                 )
 
@@ -4217,7 +4260,9 @@ def save_plugin_config():
                             nested_dict = config_dict.get(prop_key)
 
                         if isinstance(nested_dict, dict):
-                            fix_array_structures(nested_dict, prop_schema['properties'], nested_prefix)
+                            # Pass no prefix: config_dict is already the navigated sub-dict,
+                            # so path segments from the parent would mis-navigate it.
+                            fix_array_structures(nested_dict, prop_schema['properties'])
 
             # Also ensure array fields that are None get converted to empty arrays
             def ensure_array_defaults(config_dict, schema_props, prefix=''):
@@ -4277,7 +4322,8 @@ def save_plugin_config():
                                 nested_dict = config_dict[prop_key]
 
                         if isinstance(nested_dict, dict):
-                            ensure_array_defaults(nested_dict, prop_schema['properties'], nested_prefix)
+                            # Pass no prefix: config_dict is already navigated.
+                            ensure_array_defaults(nested_dict, prop_schema['properties'])
 
             if schema and 'properties' in schema:
                 # First, fix any dict structures that should be arrays
@@ -4377,6 +4423,21 @@ def save_plugin_config():
             defaults = schema_mgr.generate_default_config(plugin_id, use_cache=True)
             plugin_config = schema_mgr.merge_with_defaults(plugin_config, defaults)
 
+        # After merging defaults, replace any None array values with their schema defaults.
+        # merge_with_defaults gives user config higher priority, so a None submitted by
+        # the client can survive the merge — this pass cleans those up.
+        def _fix_none_arrays(cfg, props):
+            for k, pschema in props.items():
+                if pschema.get('type') == 'array':
+                    if isinstance(cfg, dict) and (k not in cfg or cfg[k] is None):
+                        cfg[k] = pschema.get('default', [])
+                elif pschema.get('type') == 'object' and 'properties' in pschema:
+                    if isinstance(cfg, dict) and isinstance(cfg.get(k), dict):
+                        _fix_none_arrays(cfg[k], pschema['properties'])
+
+        if schema and 'properties' in schema and isinstance(plugin_config, dict):
+            _fix_none_arrays(plugin_config, schema['properties'])
+
         # Ensure enabled state is preserved after defaults merge
         # Defaults should not overwrite an explicitly preserved enabled value
         if preserved_enabled is not None: