diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f711d50..67a4cd2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,22 @@ core, the monorepo or the registry's third-party plugins calls them: `unregister_plugin_fonts`. - `PluginManager.get_enabled_plugins`. +### Config writes + +- A power cut or crash mid-save can no longer leave `config/config.json` + truncated. `ConfigManager.save_config()` wrote the file in place; it, + `save_config_atomic()`, `save_raw_file_content()` and backup rollback now + share one writer (`atomic_write_text` in `src/config_manager_atomic.py`) + that fsyncs a temp file, renames it into place and fsyncs the directory. +- `save_config_atomic()` no longer rewrites `config_secrets.json` on every + save, only when its content changes, and rotating backups no longer re-reads + every backup. The backups themselves are unchanged: + `config/backups/config.json.backup.` plus its paired secrets + backup, five newest kept. +- A save by the root-run display service keeps the file's previous owner + instead of handing `config.json` to root, and an install path with + "secrets" in a directory name no longer makes `config.json` mode 0640. + ### Removed - **The skin system.** Skins never rendered with the current scoreboard diff --git a/src/background_data_service.py b/src/background_data_service.py index de435147..f12b3750 100644 --- a/src/background_data_service.py +++ b/src/background_data_service.py @@ -16,14 +16,15 @@ Key Features: import itertools import time +from datetime import datetime import logging import threading import requests from typing import Dict, Any, Optional, Callable, List from dataclasses import dataclass, field from enum import Enum -import queue from concurrent.futures import ThreadPoolExecutor +import pytz from src.cache_manager import CacheManager from src.common.espn_dates import ( RANGE_RETRY_SECONDS, @@ -57,7 +58,9 @@ class FetchRequest: timeout: int = 30 retry_count: int = 0 max_retries: int = 3 - priority: int = 1 # Higher number = higher priority + # Recorded but not acted on: requests go straight to the thread pool in + # submission order. Kept because plugins pass it through. + priority: int = 1 callback: Optional[Callable] = None # Callbacks from submitters that JOINED this fetch instead of starting a # duplicate one. The primary `callback` above belongs to whoever created @@ -143,7 +146,6 @@ class BackgroundDataService: self._request_seq = itertools.count() self.active_requests: Dict[str, FetchRequest] = {} self.completed_requests: Dict[str, FetchResult] = {} - self.request_queue = queue.PriorityQueue() # Thread safety self._lock = threading.RLock() @@ -187,10 +189,12 @@ class BackgroundDataService: This ensures Recent/Upcoming managers and background service use the same cache keys. """ - # Use the centralized cache key generation from CacheManager - from src.cache_manager import CacheManager - cache_manager = CacheManager() - return cache_manager.generate_sport_cache_key(sport, date_str) + # Same format as CacheManager.generate_sport_cache_key(). This used to + # build a whole CacheManager to call it -- config load, cache-dir + # probing with test writes -- on every submit without a cache_key. + if date_str is None: + date_str = datetime.now(pytz.utc).strftime('%Y%m%d') + return f"{sport}_{date_str}" def submit_fetch_request(self, sport: str, @@ -215,7 +219,8 @@ class BackgroundDataService: headers: HTTP headers timeout: Request timeout max_retries: Maximum number of retries - priority: Request priority (higher = more important) + priority: Accepted for compatibility and ignored; requests run in + submission order. callback: Optional callback function when request completes Returns: @@ -719,7 +724,9 @@ class BackgroundDataService: 'completed_requests_count': len(self.completed_requests), 'max_completed_requests': self._max_completed_requests, 'completed_requests_usage_percent': (len(self.completed_requests) / self._max_completed_requests * 100) if self._max_completed_requests > 0 else 0, - 'queue_size': self.request_queue.qsize(), + # Nothing is queued outside the executor; kept for callers + # that read the key. + 'queue_size': 0, 'last_cleanup': self._last_completed_requests_cleanup, 'cleanup_interval': self._completed_requests_cleanup_interval } diff --git a/src/cache/cache_strategy.py b/src/cache/cache_strategy.py index 7c6bfd56..66962495 100644 --- a/src/cache/cache_strategy.py +++ b/src/cache/cache_strategy.py @@ -1,7 +1,7 @@ """ Cache Strategy -Manages cache strategies for different data types with sport-specific configurations. +Manages cache strategies (TTLs) for different data types. """ import logging @@ -18,7 +18,8 @@ class CacheStrategy: Initialize cache strategy manager. Args: - config_manager: Optional ConfigManager instance for sport-specific configs + config_manager: Optional ConfigManager instance. Kept for callers + that pass one; no strategy currently reads it. logger: Optional logger instance """ self.config_manager = config_manager @@ -26,69 +27,38 @@ class CacheStrategy: def get_sport_live_interval(self, sport_key: str) -> int: """ - Get the live_update_interval for a specific sport from config. - Falls back to default values if config is not available. - + Live-data cache interval, in seconds, for a sport: 60 for every sport. + + This used to read ``live_update_interval`` from a ``_scoreboard`` + config section. Those sections belonged to the built-in scoreboards + that the plugin system replaced; plugin config is keyed by plugin id + (``football-scoreboard``), so the lookup always fell back to 60. + Args: sport_key: Sport identifier (e.g., 'nba', 'nfl') - + Returns: Live update interval in seconds """ - if not self.config_manager: - # Default intervals - all sports use 60 seconds as default - default_intervals = { - 'soccer': 60, - 'nfl': 60, - 'nhl': 60, - 'nba': 60, - 'mlb': 60, - 'milb': 60, - 'ncaa_fb': 60, - 'ncaa_baseball': 60, - 'ncaam_basketball': 60, - } - return default_intervals.get(sport_key, 60) - - try: - config = self.config_manager.config - # All sports now use _scoreboard suffix - sport_config = config.get(f"{sport_key}_scoreboard", {}) - return sport_config.get("live_update_interval", 60) # Default to 60 seconds - except (KeyError, AttributeError, TypeError) as e: - self.logger.warning("Could not get live_update_interval for %s: %s", sport_key, e, exc_info=True) - return 60 # Default to 60 seconds - + return 60 + def get_cache_strategy(self, data_type: str, sport_key: Optional[str] = None) -> Dict[str, Any]: """ Get cache strategy for different data types. - Now respects sport-specific live_update_interval configurations. - + Args: data_type: Type of data (e.g., 'live_scores', 'stocks', 'weather_current') - sport_key: Optional sport key for sport-specific intervals - + sport_key: Optional sport key; for live data it selects the + per-sport interval from :meth:`get_sport_live_interval` + instead of the generic live default. + Returns: Dictionary with cache strategy (max_age, memory_ttl, etc.) """ - # Get sport-specific live interval if provided live_interval = None if sport_key and data_type in ['sports_live', 'live_scores']: live_interval = self.get_sport_live_interval(sport_key) - - # Try to read sport-specific config for recent/upcoming - recent_interval = None - upcoming_interval = None - if self.config_manager and sport_key: - try: - # All sports now use _scoreboard suffix - sport_cfg = self.config_manager.config.get(f"{sport_key}_scoreboard", {}) - recent_interval = sport_cfg.get('recent_update_interval') - upcoming_interval = sport_cfg.get('upcoming_update_interval') - except (KeyError, AttributeError, TypeError) as e: - self.logger.debug("Could not read sport-specific recent/upcoming intervals for %s: %s", - sport_key, e, exc_info=True) - + strategies = { # Ultra time-sensitive data (live scores, current weather) 'live_scores': { @@ -122,13 +92,13 @@ class CacheStrategy: # Sports data 'sports_recent': { - 'max_age': recent_interval or 1800, # 30 minutes default; override by config - 'memory_ttl': (recent_interval or 1800) * 2, + 'max_age': 1800, # 30 minutes + 'memory_ttl': 3600, 'force_refresh': False }, 'sports_upcoming': { - 'max_age': upcoming_interval or 10800, # 3 hours default; override by config - 'memory_ttl': (upcoming_interval or 10800) * 2, + 'max_age': 10800, # 3 hours + 'memory_ttl': 21600, 'force_refresh': False }, 'sports_schedules': { @@ -206,8 +176,6 @@ class CacheStrategy: # Live sports data if any(x in key_lower for x in ['live', 'current', 'scoreboard']): - if 'soccer' in key_lower: - return 'sports_live' # Soccer live data is very time-sensitive return 'sports_live' # Weather data diff --git a/src/cache_manager.py b/src/cache_manager.py index 7d353d0f..4d80c986 100644 --- a/src/cache_manager.py +++ b/src/cache_manager.py @@ -92,14 +92,6 @@ class CacheManager: self._strategy_component = CacheStrategy(config_manager=self.config_manager, logger=self.logger) self._metrics_component = CacheMetrics(logger=self.logger) - # Keep old attributes for backward compatibility (delegated to components) - self._memory_cache = self._memory_cache_component._cache - self._memory_cache_timestamps = self._memory_cache_component._timestamps - self._cache_lock = self._memory_cache_component._lock - self._max_memory_cache_size = self._memory_cache_component._max_size - self._memory_cache_cleanup_interval = self._memory_cache_component._cleanup_interval - self._last_memory_cache_cleanup = self._memory_cache_component._last_cleanup - # Disk cleanup configuration self._disk_cleanup_interval_hours = 24 # Run cleanup every 24 hours self._disk_cleanup_interval = 3600.0 # Minimum interval between cleanups (1 hour) for throttle @@ -229,70 +221,14 @@ class CacheManager: return None def _cleanup_memory_cache(self, force: bool = False) -> int: - """ - Clean up expired entries from memory cache and enforce size limits. - - Args: - force: If True, perform cleanup regardless of time interval - + """Sweep the memory tier: drop entries older than an hour and trim it + to its size ceiling, at most once per cleanup interval unless forced. + Returns: Number of entries removed """ - now = time.time() - - # Check if cleanup is needed - if not force and (now - self._last_memory_cache_cleanup) < self._memory_cache_cleanup_interval: - return 0 - - with self._cache_lock: - removed_count = 0 - current_time = time.time() - - # Remove expired entries (entries older than 1 hour without access are considered expired) - # We use a conservative TTL of 1 hour for cleanup - max_age_for_cleanup = 3600 # 1 hour - - expired_keys = [] - for key, timestamp in list(self._memory_cache_timestamps.items()): - if isinstance(timestamp, str): - try: - timestamp = float(timestamp) - except ValueError: - timestamp = None - - if timestamp is None or (current_time - timestamp) > max_age_for_cleanup: - expired_keys.append(key) - - # Remove expired entries - for key in expired_keys: - self._memory_cache.pop(key, None) - self._memory_cache_timestamps.pop(key, None) - removed_count += 1 - - # Enforce size limit by removing oldest entries if cache is too large - if len(self._memory_cache) > self._max_memory_cache_size: - # Sort by timestamp (oldest first) - sorted_entries = sorted( - self._memory_cache_timestamps.items(), - key=lambda x: float(x[1]) if isinstance(x[1], (int, float)) else 0 - ) - - # Remove oldest entries until we're under the limit - excess_count = len(self._memory_cache) - self._max_memory_cache_size - for i in range(excess_count): - if i < len(sorted_entries): - key = sorted_entries[i][0] - self._memory_cache.pop(key, None) - self._memory_cache_timestamps.pop(key, None) - removed_count += 1 - - self._last_memory_cache_cleanup = current_time - - if removed_count > 0: - self.logger.debug(f"Memory cache cleanup: removed {removed_count} entries (current size: {len(self._memory_cache)})") - - return removed_count - + return self._memory_cache_component.cleanup(force=force) + def _get_cache_path(self, key: str) -> Optional[str]: """Get the path for a cache file.""" return self._disk_cache_component.get_cache_path(key) @@ -412,56 +348,58 @@ class CacheManager: current_time = time.time() try: - with self._cache_lock: - for filename in os.listdir(self.cache_dir): - if not filename.endswith('.json'): - continue + # No lock: this is disk-only work, and the memory-tier lock it used + # to hold would stall every get/set while thousands of files are + # stat'd. A file deleted mid-scan is skipped below. + for filename in os.listdir(self.cache_dir): + if not filename.endswith('.json'): + continue + + # Extract key from filename (remove .json extension) + key = filename[:-5] # Remove '.json' + + file_path = os.path.join(self.cache_dir, filename) + + try: + # Get file stats + stat_info = os.stat(file_path) + size_bytes = stat_info.st_size + modified_time = stat_info.st_mtime + age_seconds = current_time - modified_time - # Extract key from filename (remove .json extension) - key = filename[:-5] # Remove '.json' + # Format age display + if age_seconds < 60: + age_display = f"{int(age_seconds)}s" + elif age_seconds < 3600: + age_display = f"{int(age_seconds / 60)}m" + elif age_seconds < 86400: + age_display = f"{int(age_seconds / 3600)}h" + else: + age_display = f"{int(age_seconds / 86400)}d" - file_path = os.path.join(self.cache_dir, filename) + # Format size display + if size_bytes < 1024: + size_display = f"{size_bytes}B" + elif size_bytes < 1024 * 1024: + size_display = f"{size_bytes / 1024:.1f}KB" + else: + size_display = f"{size_bytes / (1024 * 1024):.1f}MB" + + cache_files.append({ + 'key': key, + 'filename': filename, + 'age_seconds': age_seconds, + 'age_display': age_display, + 'size_bytes': size_bytes, + 'size_display': size_display, + 'path': file_path, + 'modified_time': modified_time, + 'modified_datetime': datetime.fromtimestamp(modified_time).isoformat() + }) + except OSError as e: + self.logger.warning(f"Error getting stats for cache file {filename} at {file_path}: {e}", exc_info=True) + continue - try: - # Get file stats - stat_info = os.stat(file_path) - size_bytes = stat_info.st_size - modified_time = stat_info.st_mtime - age_seconds = current_time - modified_time - - # Format age display - if age_seconds < 60: - age_display = f"{int(age_seconds)}s" - elif age_seconds < 3600: - age_display = f"{int(age_seconds / 60)}m" - elif age_seconds < 86400: - age_display = f"{int(age_seconds / 3600)}h" - else: - age_display = f"{int(age_seconds / 86400)}d" - - # Format size display - if size_bytes < 1024: - size_display = f"{size_bytes}B" - elif size_bytes < 1024 * 1024: - size_display = f"{size_bytes / 1024:.1f}KB" - else: - size_display = f"{size_bytes / (1024 * 1024):.1f}MB" - - cache_files.append({ - 'key': key, - 'filename': filename, - 'age_seconds': age_seconds, - 'age_display': age_display, - 'size_bytes': size_bytes, - 'size_display': size_display, - 'path': file_path, - 'modified_time': modified_time, - 'modified_datetime': datetime.fromtimestamp(modified_time).isoformat() - }) - except OSError as e: - self.logger.warning(f"Error getting stats for cache file {filename} at {file_path}: {e}", exc_info=True) - continue - except OSError as e: self.logger.error(f"Error listing cache directory {self.cache_dir}: {e}", exc_info=True) return [] @@ -1001,14 +939,7 @@ class CacheManager: Returns: Dictionary with memory cache statistics """ - with self._cache_lock: - return { - 'size': len(self._memory_cache), - 'max_size': self._max_memory_cache_size, - 'usage_percent': (len(self._memory_cache) / self._max_memory_cache_size * 100) if self._max_memory_cache_size > 0 else 0, - 'last_cleanup': self._last_memory_cache_cleanup, - 'cleanup_interval': self._memory_cache_cleanup_interval - } + return self._memory_cache_component.get_stats() def log_memory_cache_stats(self) -> None: """Log current memory cache statistics.""" diff --git a/src/config_manager.py b/src/config_manager.py index f544f35c..d9800d4d 100644 --- a/src/config_manager.py +++ b/src/config_manager.py @@ -7,9 +7,11 @@ files: * ``config/config.json`` — main user-editable configuration. * ``config/config_secrets.json`` — sensitive values (API keys, tokens). -All writes go through :class:`~src.config_manager_atomic.AtomicConfigManager` -which performs a backup before overwriting, validates the result, and rolls -back on error. This makes config corruption essentially impossible. +Every write of either file goes through +:func:`~src.config_manager_atomic.atomic_write_text`: temp file, fsync, +rename, directory fsync. A crash or power cut mid-save leaves the old file or +the new one, never a truncated one. :meth:`ConfigManager.save_config_atomic` +additionally keeps rotating backups in ``config/backups/``. Plugin configuration -------------------- @@ -34,13 +36,11 @@ from src.exceptions import ConfigError from src.logging_config import get_logger from src.config_manager_atomic import ( AtomicConfigManager, SaveResult, SaveResultStatus, - BackupInfo, ValidationResult + BackupInfo, ValidationResult, atomic_write_json ) from src.common.permission_utils import ( ensure_directory_permissions, - ensure_file_permissions, ensure_shared_group_ownership, - get_config_file_mode, get_config_dir_mode ) @@ -114,11 +114,12 @@ class ConfigManager: # Strip secrets from main config before saving config_to_write = self._strip_secrets_recursive(new_config_data, secrets_content) - # Use atomic manager to save + # The secrets file is only read here, never changed, so it is not + # handed over for rewriting. atomic_mgr = self._get_atomic_manager() result = atomic_mgr.save_config_atomic( new_config=config_to_write, - new_secrets=secrets_content if secrets_content else None, + new_secrets=None, create_backup=create_backup, validate_after_write=validate_after_write ) @@ -354,9 +355,8 @@ class ConfigManager: config_to_write = self._strip_secrets_recursive(new_config_data, secrets_content) try: - with open(self.config_path, 'w') as f: - json.dump(config_to_write, f, indent=4) - + atomic_write_json(self.config_path, config_to_write) + # Update the in-memory config to the new state (which includes secrets for runtime) self.config = new_config_data self._loaded_sig = self._files_signature() @@ -438,14 +438,8 @@ class ConfigManager: with open(self.template_path, 'r') as template_file: template_data = json.load(template_file) - with open(self.config_path, 'w') as config_file: - json.dump(template_data, config_file, indent=4) - - # Set proper file permissions after creation - config_path_obj = Path(self.config_path) - ensure_file_permissions(config_path_obj, get_config_file_mode(config_path_obj)) - ensure_shared_group_ownership(config_path_obj) - + atomic_write_json(self.config_path, template_data) + self.logger.info(f"Created config.json from template at {os.path.abspath(self.config_path)}") def _migrate_config(self) -> None: @@ -600,54 +594,10 @@ class ConfigManager: path_obj = Path(path_to_save) ensure_directory_permissions(path_obj.parent, get_config_dir_mode()) - # Use atomic write: write to temp file first, then move atomically - # This works even if the existing file isn't writable (as long as directory is writable) - import tempfile - file_mode = get_config_file_mode(path_obj) - - # Create temp file in same directory to ensure atomic move works - temp_fd, temp_path = tempfile.mkstemp( - suffix='.json', - dir=str(path_obj.parent), - text=True - ) - - try: - # Write to temp file - with os.fdopen(temp_fd, 'w', encoding='utf-8') as f: - json.dump(data, f, indent=4) - f.flush() - os.fsync(f.fileno()) - - # Set permissions on temp file before moving - try: - os.chmod(temp_path, file_mode) - except OSError: - pass # Non-critical if chmod fails - - # Atomically move temp file to final location - # This works even if target file exists and isn't writable - os.replace(temp_path, str(path_obj)) - temp_path = None # Mark as moved so we don't try to clean it up - - # Ensure final file has correct permissions - try: - ensure_file_permissions(path_obj, file_mode) - ensure_shared_group_ownership(path_obj) - except OSError as perm_error: - # If we can't set permissions but file was written, log warning but don't fail - self.logger.warning( - f"File {path_to_save} was written successfully but could not set permissions: {perm_error}. " - f"This may cause issues if the file needs to be accessible by other users." - ) - finally: - # Clean up temp file if it still exists (move failed) - if temp_path and os.path.exists(temp_path): - try: - os.remove(temp_path) - except OSError: - pass - + # A rename, not an in-place write, so this works even when the + # existing file isn't writable (as long as the directory is). + atomic_write_json(path_obj, data) + self.logger.info(f"{file_type.capitalize()} configuration successfully saved to {os.path.abspath(path_to_save)}") # If we just saved the main config or secrets, the merged self.config might be stale. diff --git a/src/config_manager_atomic.py b/src/config_manager_atomic.py index a8d712c6..f0d90181 100644 --- a/src/config_manager_atomic.py +++ b/src/config_manager_atomic.py @@ -10,15 +10,16 @@ import os import re import shutil import tempfile +import time from datetime import datetime, timedelta from pathlib import Path -from typing import Dict, Any, Optional, List, Tuple +from typing import Dict, Any, Optional, List, Tuple, Union from dataclasses import dataclass from enum import Enum from src.exceptions import ConfigError from src.logging_config import get_logger -from src.common.permission_utils import ensure_shared_group_ownership +from src.common.permission_utils import ensure_shared_group_ownership, get_config_file_mode # Version stamp in a backup's filename: config.json.backup.. BACKUP_VERSION_FORMAT = "%Y%m%d_%H%M%S_%f" @@ -33,6 +34,98 @@ LEGACY_BACKUP_VERSION_FORMAT = "%Y%m%d_%H%M%S" # mistaken for one and silently mis-parsed. _BACKUP_COLLISION_SUFFIX_RE = re.compile(r"^(?P.+)-(?P\d+)$") +# Windows refuses to rename over a file another process has open (a reader +# mid-load). Linux never does, so this only ever retries on a dev machine. +_WINDOWS_REPLACE_ATTEMPTS = 10 +_WINDOWS_REPLACE_DELAY = 0.05 + + +def _replace(source: Path, destination: Path) -> None: + for attempt in range(_WINDOWS_REPLACE_ATTEMPTS): + try: + os.replace(source, destination) + return + except PermissionError: + if os.name != 'nt' or attempt == _WINDOWS_REPLACE_ATTEMPTS - 1: + raise + time.sleep(_WINDOWS_REPLACE_DELAY) + + +def _fsync_directory(directory: Path) -> None: + """Persist a rename: until the directory entry itself is on disk, a power + cut can bring back the old file, or on some filesystems neither. Windows + can't open a directory for fsync, and NTFS journals renames anyway.""" + if os.name == 'nt': + return + try: + fd = os.open(directory, os.O_RDONLY) + except OSError: + return + try: + os.fsync(fd) + except OSError: + pass + finally: + os.close(fd) + + +def atomic_write_text(path: Union[str, Path], text: str, mode: Optional[int] = None) -> None: + """ + Replace ``path`` with ``text`` so that a crash or power cut at any point + leaves either the old file or the new one, never a truncated mix. + + The data goes to a temp file in the same directory, is fsynced, and is + renamed over the target; the directory is then fsynced so the rename + itself survives. The temp file gets its final mode (0o644, or 0o640 when + the file name contains "secrets"; a directory name doesn't count) before + the rename, so no reader ever sees mkstemp's 0o600. + + A rename hands the file to whoever wrote it. When running as root (the + display service) the previous owner is copied onto the temp file first, + so a root save doesn't leave the web user's config.json owned by root; + the group is then moved to the shared one (ensure_shared_group_ownership) + as before. On failure the temp file is removed, the target is untouched, + and the error propagates. + """ + path = Path(path) + if mode is None: + mode = get_config_file_mode(Path(path.name)) + try: + previous = path.stat() + except OSError: + previous = None + + fd, temp_name = tempfile.mkstemp(dir=str(path.parent), prefix=f".{path.name}.tmp.") + temp_path = Path(temp_name) + try: + with os.fdopen(fd, 'wb') as f: + f.write(text.encode('utf-8')) + f.flush() + os.fsync(f.fileno()) + if previous is not None and hasattr(os, 'geteuid') and os.geteuid() == 0: + try: + os.chown(temp_path, previous.st_uid, previous.st_gid) + except OSError: + pass + os.chmod(temp_path, mode) + _replace(temp_path, path) + except BaseException: + try: + temp_path.unlink() + except OSError: + pass + raise + + ensure_shared_group_ownership(path) + _fsync_directory(path.parent) + + +def atomic_write_json(path: Union[str, Path], data: Any, mode: Optional[int] = None) -> None: + """Serialize ``data`` the way every config file is written (indent=4) and + write it with :func:`atomic_write_text`. Serialization happens first, so + a value json can't encode fails before anything on disk changes.""" + atomic_write_text(path, json.dumps(data, indent=4), mode) + class SaveResultStatus(Enum): """Status of a save operation.""" @@ -75,11 +168,11 @@ class AtomicConfigManager: Manages atomic configuration saves with backup and rollback support. Provides: - - Atomic file writes (write to temp, validate, atomic move) + - Durable atomic file writes (see atomic_write_text) - Automatic backups before saves - Backup rotation (keep last N backups) - Rollback functionality - - Post-write validation + - Validation before the write """ def __init__( @@ -94,7 +187,8 @@ class AtomicConfigManager: Args: config_path: Path to main configuration file - secrets_path: Optional path to secrets file (saved atomically with main config) + secrets_path: Optional path to secrets file (backed up with the main + config, and rewritten by a save only when its content changes) backup_dir: Directory to store backups (default: config/backups/) max_backups: Maximum number of backups to keep """ @@ -126,24 +220,25 @@ class AtomicConfigManager: Process: 1. Create backup if requested - 2. Write to temporary files - 3. Validate written files - 4. Atomically move temp files to final locations - 5. If validation fails, rollback - + 2. Serialize and validate the new content in memory + 3. Write each file with atomic_write_text (temp file, fsync, rename) + + The secrets file is only rewritten when ``new_secrets`` differs from + what is already on disk. + Args: new_config: New configuration data for main config file new_secrets: Optional new secrets data create_backup: Whether to create backup before saving - validate_after_write: Whether to validate after writing - + validate_after_write: Whether to validate the content before it + replaces the config file + Returns: SaveResult with status and details """ backup_path = None - + try: - # Step 1: Create backup if requested if create_backup: backup_result = self._create_backup() if backup_result: @@ -151,35 +246,23 @@ class AtomicConfigManager: self.logger.info(f"Created backup: {backup_path}") else: self.logger.warning("Failed to create backup, continuing with save") - - # Step 2: Write to temporary files - temp_config_path, temp_secrets_path = self._write_to_temp_files( - new_config, new_secrets - ) - - # Step 3: Validate written files + + config_text, secrets_text = self._serialize(new_config, new_secrets) + if validate_after_write: - validation_result = self._validate_config_file(temp_config_path) + validation_result = self._validate_config_text(config_text) if not validation_result.is_valid: - # Clean up temp files - self._cleanup_temp_files(temp_config_path, temp_secrets_path) - - # Rollback if backup was created - if backup_path: - self._rollback_from_backup(backup_path) - return SaveResult( status=SaveResultStatus.VALIDATION_FAILED, message="Configuration validation failed after write", backup_path=backup_path, validation_errors=validation_result.errors ) - - # Step 4: Atomically move temp files to final locations - self._atomic_move(temp_config_path, self.config_path) - if temp_secrets_path and self.secrets_path: - self._atomic_move(temp_secrets_path, self.secrets_path) - + + atomic_write_text(self.config_path, config_text) + if secrets_text is not None: + atomic_write_text(self.secrets_path, secrets_text) + self.logger.info(f"Configuration saved atomically to {self.config_path}") return SaveResult( @@ -256,57 +339,60 @@ class AtomicConfigManager: List of BackupInfo objects, sorted by timestamp (newest first) """ backups = [] - - if not self.backup_dir.exists(): - return backups - - # Look for backup files (format: config.json.backup.) - config_name = self.config_path.name - backup_pattern = f"{config_name}.backup.*" - - for backup_file in self.backup_dir.glob(backup_pattern): + for version, backup_file, timestamp in self._backup_entries(): try: - # The version reported here is what rollback_config() matches - # against, so it has to be the exact string in the filename. - # - # It did not used to be. This read .stem, which drops only the - # last dot-component, so for config.json.backup.20240101_120000 - # parts was ['config', 'json', 'backup'] and parts[-2] was - # 'json' -- never 'backup'. The filename branch could not be - # reached, every backup fell through to the mtime fallback, and - # the version was a second-granularity restamp of the mtime - # rather than the name on disk. Two backups a second apart could - # therefore report the same version, and rollback would pick - # whichever the glob happened to yield first. - # Strip the exact prefix the glob just matched, so a config - # whose own name contains '.backup.' can't shift the split. - timestamp_str = backup_file.name[len(f"{config_name}.backup."):] - timestamp = self._parse_backup_version(timestamp_str) - if timestamp is None: - # Not a version this code wrote (hand-copied, renamed). - # Order it by mtime, but keep the on-disk version string so - # it can still be named in a rollback. - timestamp = datetime.fromtimestamp(backup_file.stat().st_mtime) - - # Validate backup file - is_valid = self._validate_backup_file(backup_file) - - backup_info = BackupInfo( - version=timestamp_str, + backups.append(BackupInfo( + version=version, path=str(backup_file), timestamp=timestamp, size=backup_file.stat().st_size, - is_valid=is_valid - ) - backups.append(backup_info) - + is_valid=self._validate_backup_file(backup_file) + )) except Exception as e: self.logger.warning(f"Error reading backup {backup_file}: {e}") - - # Sort by timestamp (newest first) - backups.sort(key=lambda b: b.timestamp, reverse=True) - return backups + + def _backup_entries(self) -> List[Tuple[str, Path, datetime]]: + """ + ``(version, path, timestamp)`` for every ``config.json.backup.`` + in the backup directory, newest first. Names only -- no file is opened, + so rotation can call this on every save without re-parsing each backup. + """ + entries = [] + if not self.backup_dir.exists(): + return entries + + prefix = f"{self.config_path.name}.backup." + for backup_file in self.backup_dir.glob(f"{prefix}*"): + # The version reported here is what rollback_config() matches + # against, so it has to be the exact string in the filename. + # + # It did not used to be. This read .stem, which drops only the + # last dot-component, so for config.json.backup.20240101_120000 + # parts was ['config', 'json', 'backup'] and parts[-2] was + # 'json' -- never 'backup'. The filename branch could not be + # reached, every backup fell through to the mtime fallback, and + # the version was a second-granularity restamp of the mtime + # rather than the name on disk. Two backups a second apart could + # therefore report the same version, and rollback would pick + # whichever the glob happened to yield first. + # Strip the exact prefix the glob just matched, so a config + # whose own name contains '.backup.' can't shift the split. + version = backup_file.name[len(prefix):] + timestamp = self._parse_backup_version(version) + if timestamp is None: + # Not a version this code wrote (hand-copied, renamed). + # Order it by mtime, but keep the on-disk version string so + # it can still be named in a rollback. + try: + timestamp = datetime.fromtimestamp(backup_file.stat().st_mtime) + except OSError as e: + self.logger.warning(f"Error reading backup {backup_file}: {e}") + continue + entries.append((version, backup_file, timestamp)) + + entries.sort(key=lambda entry: entry[2], reverse=True) + return entries @staticmethod def _parse_backup_version(version: str) -> Optional[datetime]: @@ -417,100 +503,37 @@ class AtomicConfigManager: self.logger.error(f"Error creating backup: {e}", exc_info=True) return None - def _write_to_temp_files( + def _serialize( self, config_data: Dict[str, Any], secrets_data: Optional[Dict[str, Any]] = None - ) -> Tuple[Path, Optional[Path]]: + ) -> Tuple[str, Optional[str]]: """ - Write configuration data to temporary files. + Serialize both files before either is written, so a value json can't + encode fails the save before anything on disk changes. Returns: - Tuple of (temp_config_path, temp_secrets_path) - """ - # Create temp file in same directory as config (for atomic move) - temp_config = tempfile.NamedTemporaryFile( - mode='w', - dir=self.config_path.parent, - prefix=f".{self.config_path.name}.tmp.", - delete=False, - suffix='.json' - ) - temp_config_path = Path(temp_config.name) - - try: - json.dump(config_data, temp_config, indent=4) - temp_config.close() - except Exception as e: - temp_config.close() - if temp_config_path.exists(): - temp_config_path.unlink() - raise ConfigError(f"Error writing temp config file: {e}") from e - - # Write secrets to temp file if provided - temp_secrets_path = None - if secrets_data is not None and self.secrets_path: - temp_secrets = tempfile.NamedTemporaryFile( - mode='w', - dir=self.secrets_path.parent, - prefix=f".{self.secrets_path.name}.tmp.", - delete=False, - suffix='.json' - ) - temp_secrets_path = Path(temp_secrets.name) - - try: - json.dump(secrets_data, temp_secrets, indent=4) - temp_secrets.close() - except Exception as e: - temp_secrets.close() - if temp_secrets_path.exists(): - temp_secrets_path.unlink() - # Clean up config temp file too - if temp_config_path.exists(): - temp_config_path.unlink() - raise ConfigError(f"Error writing temp secrets file: {e}") from e - - return temp_config_path, temp_secrets_path - - def _atomic_move(self, source: Path, destination: Path) -> None: - """ - Atomically move a file (rename operation). - - On most filesystems, rename is atomic, which prevents corruption - if the process is interrupted. - - Sets appropriate file permissions after move to ensure service can read config. + Tuple of (config_text, secrets_text); secrets_text is None when + there is no secrets file to write or its content is unchanged. """ try: - # Ensure destination directory exists - destination.parent.mkdir(parents=True, exist_ok=True) - - # Determine target permissions based on file type - # config.json should be 644 (readable by all, including root service) - # config_secrets.json should be 640 (readable by owner and group) - if 'secrets' in str(destination): - target_mode = 0o640 # rw-r----- - else: - target_mode = 0o644 # rw-r--r-- - - # Atomic move (rename) - source.replace(destination) - - # Set permissions after move to ensure they're correct - # This is important because temp files may have different permissions - # and we need root service to be able to read config.json - os.chmod(destination, target_mode) + config_text = json.dumps(config_data, indent=4) + except (TypeError, ValueError) as e: + raise ConfigError(f"Error serializing config: {e}") from e - # Also fix group ownership when this save is running as root - # (the display service): 0o640 alone only helps the non-root web - # user read a root-written secrets file if its group already - # matches the web user's group, which isn't guaranteed. See - # permission_utils.ensure_shared_group_ownership for why. - ensure_shared_group_ownership(destination) - - except Exception as e: - raise ConfigError(f"Error during atomic move: {e}") from e + if secrets_data is None or not self.secrets_path or self._secrets_unchanged(secrets_data): + return config_text, None + try: + return config_text, json.dumps(secrets_data, indent=4) + except (TypeError, ValueError) as e: + raise ConfigError(f"Error serializing secrets: {e}") from e + + def _secrets_unchanged(self, secrets_data: Dict[str, Any]) -> bool: + try: + with open(self.secrets_path, 'r') as f: + return json.load(f) == secrets_data + except (OSError, ValueError): + return False def _validate_config_file(self, config_path: Path) -> ValidationResult: """ @@ -521,30 +544,38 @@ class AtomicConfigManager: - Valid JSON format - Can be parsed successfully """ - errors = [] - warnings = [] - if not config_path.exists(): - errors.append(f"Config file does not exist: {config_path}") - return ValidationResult(is_valid=False, errors=errors, warnings=warnings) - + return ValidationResult( + is_valid=False, + errors=[f"Config file does not exist: {config_path}"], + warnings=[] + ) try: with open(config_path, 'r') as f: - data = json.load(f) - - # Basic validation: should be a dict + text = f.read() + except Exception as e: + return ValidationResult( + is_valid=False, + errors=[f"Error reading config file: {str(e)}"], + warnings=[] + ) + return self._validate_config_text(text) + + @staticmethod + def _validate_config_text(text: str) -> ValidationResult: + """Validate serialized configuration: parseable JSON holding an object.""" + errors = [] + warnings = [] + try: + data = json.loads(text) if not isinstance(data, dict): errors.append("Configuration must be a JSON object") - - # Check file is not empty if not data: warnings.append("Configuration file is empty") - except json.JSONDecodeError as e: errors.append(f"Invalid JSON: {str(e)}") except Exception as e: errors.append(f"Error reading config file: {str(e)}") - return ValidationResult( is_valid=len(errors) == 0, errors=errors, @@ -563,6 +594,9 @@ class AtomicConfigManager: """ Rollback configuration from a backup file. + The backup is written back with atomic_write_text, so a failure + partway through a restore can't truncate the live config either. + Args: backup_path: Path to backup file to restore @@ -575,71 +609,50 @@ class AtomicConfigManager: self.logger.error(f"Backup file not found: {backup_path}") return False - # Validate backup before restoring - if not self._validate_backup_file(backup_file): + try: + with open(backup_file, 'r') as f: + config_text = f.read() + except Exception as e: + self.logger.error(f"Error reading backup {backup_path}: {e}", exc_info=True) + return False + + if not self._validate_config_text(config_text).is_valid: self.logger.error(f"Backup file is invalid: {backup_path}") return False try: - # Restore main config - shutil.copy2(backup_file, self.config_path) + atomic_write_text(self.config_path, config_text) self.logger.info(f"Restored config from backup: {backup_path}") - # Try to restore secrets backup if it exists - if self.secrets_path: - # Look for corresponding secrets backup - # Format: config_secrets.json.backup.TIMESTAMP - backup_name = backup_file.name - if '.backup.' in backup_name: - timestamp = backup_name.split('.backup.')[-1] - secrets_backup_name = f"{self.secrets_path.name}.backup.{timestamp}" - secrets_backup_path = self.backup_dir / secrets_backup_name - - if secrets_backup_path.exists(): - shutil.copy2(secrets_backup_path, self.secrets_path) - self.logger.info(f"Restored secrets from backup: {secrets_backup_path}") + secrets_backup_path = self._paired_secrets_backup(backup_file) + if secrets_backup_path is not None and secrets_backup_path.exists(): + with open(secrets_backup_path, 'r') as f: + atomic_write_text(self.secrets_path, f.read()) + self.logger.info(f"Restored secrets from backup: {secrets_backup_path}") return True except Exception as e: self.logger.error(f"Error during rollback: {e}", exc_info=True) return False + + def _paired_secrets_backup(self, backup_file: Path) -> Optional[Path]: + """The config_secrets.json.backup. taken alongside a config backup.""" + if not self.secrets_path or '.backup.' not in backup_file.name: + return None + version = backup_file.name.split('.backup.')[-1] + return self.backup_dir / f"{self.secrets_path.name}.backup.{version}" def _rotate_backups(self) -> None: """Remove old backups, keeping only the most recent N backups.""" - backups = self.list_backups() - - if len(backups) <= self.max_backups: - return - - # Sort by timestamp (oldest first) and remove excess - backups.sort(key=lambda b: b.timestamp) - backups_to_remove = backups[:-self.max_backups] - - for backup in backups_to_remove: + for _, backup_file, _ in self._backup_entries()[self.max_backups:]: try: - Path(backup.path).unlink() - self.logger.debug(f"Removed old backup: {backup.path}") + backup_file.unlink() + self.logger.debug(f"Removed old backup: {backup_file}") - # Also remove corresponding secrets backup if it exists - if self.secrets_path: - backup_name = Path(backup.path).name - if '.backup.' in backup_name: - timestamp = backup_name.split('.backup.')[-1] - secrets_backup_name = f"{self.secrets_path.name}.backup.{timestamp}" - secrets_backup_path = self.backup_dir / secrets_backup_name - if secrets_backup_path.exists(): - secrets_backup_path.unlink() + secrets_backup_path = self._paired_secrets_backup(backup_file) + if secrets_backup_path is not None and secrets_backup_path.exists(): + secrets_backup_path.unlink() except Exception as e: - self.logger.warning(f"Error removing old backup {backup.path}: {e}") - - def _cleanup_temp_files(self, *temp_paths: Path) -> None: - """Clean up temporary files.""" - for temp_path in temp_paths: - if temp_path and temp_path.exists(): - try: - temp_path.unlink() - except Exception as e: - self.logger.warning(f"Error cleaning up temp file {temp_path}: {e}") - + self.logger.warning(f"Error removing old backup {backup_file}: {e}") diff --git a/test/test_background_data_service.py b/test/test_background_data_service.py index 587cab30..ef066d7c 100644 --- a/test/test_background_data_service.py +++ b/test/test_background_data_service.py @@ -297,3 +297,71 @@ class TestGetBackgroundService: shutdown_background_service() with pytest.raises(ValueError): get_background_service() + + +# --------------------------------------------------------------------------- +# Sport cache keys +# --------------------------------------------------------------------------- + +class _FrozenDatetime: + """Stands in for the datetime class at a fixed instant. now() without a + tz answers in a UTC-4 local zone, so a key built from local time shows.""" + + def __init__(self, moment): + self._moment = moment + + def now(self, tz=None): + from datetime import timedelta, timezone + if tz is None: + return self._moment.astimezone(timezone(timedelta(hours=-4))).replace(tzinfo=None) + return self._moment.astimezone(tz) + + +class TestSportCacheKey: + """get_sport_cache_key() must produce CacheManager's key format without + building a CacheManager (config load + cache-dir probing) to do it.""" + + @pytest.mark.parametrize("sport,date_str", [ + ("nfl", "20260922"), ("ncaa_fb", "20251231"), ("soccer", ""), + ]) + def test_explicit_date_matches_cache_manager(self, service, sport, date_str): + from src.cache_manager import CacheManager + expected = CacheManager.generate_sport_cache_key(None, sport, date_str) + assert service.get_sport_cache_key(sport, date_str) == expected + + def test_default_date_is_today_in_utc_like_cache_manager(self, service): + from datetime import datetime, timezone + import src.cache_manager as cm_module + # 23:30 on the 21st in New York is already the 22nd in UTC. + frozen = _FrozenDatetime(datetime(2026, 9, 22, 3, 30, tzinfo=timezone.utc)) + with patch.object(bds_module, "datetime", frozen), \ + patch.object(cm_module, "datetime", frozen): + ours = service.get_sport_cache_key("nba") + theirs = cm_module.CacheManager.generate_sport_cache_key(None, "nba") + assert ours == theirs == "nba_20260922" + + def test_does_not_construct_a_cache_manager(self, service): + with patch("src.cache_manager.CacheManager.__init__", + side_effect=AssertionError("CacheManager constructed")): + assert service.get_sport_cache_key("nhl", "20260101") == "nhl_20260101" + assert service.get_sport_cache_key("nhl").startswith("nhl_") + + def test_submit_without_cache_key_looks_up_todays_sport_key( + self, service, mock_cache_manager): + mock_cache_manager.get.return_value = {"events": []} + with patch("src.cache_manager.CacheManager.__init__", + side_effect=AssertionError("CacheManager constructed")): + service.submit_fetch_request("mlb", 2026, "http://example.invalid/x") + key = mock_cache_manager.get.call_args[0][0] + assert key == service.get_sport_cache_key("mlb") + + +class TestPriorityIsAcceptedAndIgnored: + def test_priority_keyword_is_accepted(self, service, mock_cache_manager): + mock_cache_manager.get.return_value = {"cached": True} + rid = service.submit_fetch_request( + "nfl", 2026, "http://example.invalid/x", cache_key="k", priority=5) + assert service.get_result(rid).cached is True + + def test_statistics_still_report_an_empty_queue(self, service): + assert service.get_statistics()["queue_size"] == 0 diff --git a/test/test_cache_manager_memory_tier.py b/test/test_cache_manager_memory_tier.py new file mode 100644 index 00000000..af6f2f20 --- /dev/null +++ b/test/test_cache_manager_memory_tier.py @@ -0,0 +1,118 @@ +"""CacheManager's memory tier is MemoryCache's, not a copy of it. + +CacheManager used to re-implement MemoryCache.cleanup() line for line and +read the component's private dicts and lock through aliases bound at +construction. Those aliases went stale the moment the component was replaced +(tests do exactly that), and the listing of the cache *directory* held the +memory tier's lock for the whole scan. +""" + +import os +import time +from unittest.mock import patch + +import pytest + +from src.cache.memory_cache import MemoryCache +from src.cache_manager import CacheManager + + +@pytest.fixture +def cm(tmp_path): + with patch('src.cache_manager.CacheManager._get_writable_cache_dir', + return_value=str(tmp_path)): + manager = CacheManager() + # The disk sweep thread stats this directory too; keep it out of the + # os.stat spies below. + manager.stop_cleanup_thread() + yield manager + + +def test_cleanup_and_stats_follow_a_replaced_component(cm): + """Replace the component the way test_cache_ttl_honoured does; cleanup and + stats must act on the new one, not on dicts captured at construction.""" + cm._memory_cache_component = MemoryCache(max_size=7, cleanup_interval=11.0) + cm._memory_cache_component.set("fresh", {"v": 1}) + cm._memory_cache_component.set("stale", {"v": 2}) + cm._memory_cache_component._timestamps["stale"] = time.time() - 4000 + + assert cm._cleanup_memory_cache(force=True) == 1 + assert cm._memory_cache_component.get("stale") is None + assert cm._memory_cache_component.get("fresh") == {"v": 1} + + stats = cm.get_memory_cache_stats() + assert stats["size"] == 1 + assert stats["max_size"] == 7 + assert stats["cleanup_interval"] == 11.0 + assert stats["usage_percent"] == pytest.approx(100 / 7) + + +def test_periodic_cleanup_is_throttled_and_records_its_run(cm): + mem = cm._memory_cache_component + mem.set("stale", {"v": 1}) + mem._timestamps["stale"] = time.time() - 4000 + + # Within the interval: nothing runs, even through the get path. + assert cm._cleanup_memory_cache() == 0 + assert mem.size() == 1 + + mem._last_cleanup = time.time() - mem._cleanup_interval - 1 + before = time.time() + cm.get_cached_data("missing") # triggers the periodic sweep + assert mem.size() == 0 + assert cm.get_memory_cache_stats()["last_cleanup"] >= before + + +def test_stats_have_the_documented_shape(cm): + cm.set("k", {"v": 1}) + stats = cm.get_memory_cache_stats() + assert set(stats) == {"size", "max_size", "usage_percent", + "last_cleanup", "cleanup_interval"} + assert stats["size"] == 1 + assert stats["max_size"] == cm._memory_cache_component.max_size() + + +def test_listing_the_cache_dir_does_not_hold_the_memory_lock(cm, tmp_path): + """8,864 files on a real rig: every get/set used to wait out the scan.""" + for name in ("a", "b"): + (tmp_path / f"{name}.json").write_text("{}") + (tmp_path / "notes.txt").write_text("x") + + lock = cm._memory_cache_component._lock + held_during_stat = [] + real_stat = os.stat + + def spying_stat(path, *args, **kwargs): + held_during_stat.append(lock.locked()) + return real_stat(path, *args, **kwargs) + + with patch('src.cache_manager.os.stat', side_effect=spying_stat): + files = cm.list_cache_files() + + assert held_during_stat and not any(held_during_stat) + assert sorted(f["key"] for f in files) == ["a", "b"] + + +def test_listing_skips_a_file_deleted_mid_scan(cm, tmp_path): + for name in ("a", "b"): + (tmp_path / f"{name}.json").write_text("{}") + real_stat = os.stat + + def vanishing_stat(path, *args, **kwargs): + if str(path).endswith("a.json"): + raise FileNotFoundError(path) + return real_stat(path, *args, **kwargs) + + with patch('src.cache_manager.os.stat', side_effect=vanishing_stat): + files = cm.list_cache_files() + + assert [f["key"] for f in files] == ["b"] + + +def test_listing_is_newest_first(cm, tmp_path): + now = time.time() + for i, name in enumerate(("old", "mid", "new")): + p = tmp_path / f"{name}.json" + p.write_text("{}") + os.utime(p, (now - 300 + i * 100, now - 300 + i * 100)) + assert [f["key"] for f in cm.list_cache_files()] == ["new", "mid", "old"] diff --git a/test/test_cache_strategy_intervals.py b/test/test_cache_strategy_intervals.py new file mode 100644 index 00000000..ef489973 --- /dev/null +++ b/test/test_cache_strategy_intervals.py @@ -0,0 +1,136 @@ +"""CacheStrategy intervals, pinned across the whole input grid. + +The strategy table used to carry a per-sport defaults dict whose every value +was 60, and a soccer branch identical to its else. These tests pin the +returned strategy for every data type x sport key x config shape, so +simplifying the lookup cannot change what any caller gets back. They were +written against the pre-cleanup code and pass on it unchanged, except for +the legacy `_scoreboard` config shape (see below), which that code +still read. +""" + +import pytest + +from src.cache.cache_strategy import CacheStrategy + + +class _Cfg: + def __init__(self, config): + self.config = config + + +class _NoConfigAttr: + pass + + +# Plugin config sections are keyed by plugin id. Their intervals belong to the +# plugin, and the strategy table has never read them. +_PLUGIN_ID_CONFIG = { + pid: {"live_update_interval": 5, "recent_update_interval": 7, + "upcoming_update_interval": 9} + for pid in ("football-scoreboard", "basketball-scoreboard", + "baseball-scoreboard", "hockey-scoreboard", "soccer-scoreboard") +} + +# `_scoreboard` sections come from the built-in scoreboards the plugin +# system replaced. An install upgraded from that era can still carry them in +# config.json (nothing deletes them). No current caller passes a sport key to +# the strategy, but a stale section must not steer cache TTLs if one does. +_LEGACY_SCOREBOARD_CONFIG = { + f"{sport}_scoreboard": {"live_update_interval": 5, + "recent_update_interval": 7, + "upcoming_update_interval": 9} + for sport in ("nfl", "nba", "mlb", "nhl", "soccer", "ncaa_fb", + "ncaa_baseball", "ncaam_basketball", "milb") +} + +CONFIG_MANAGERS = { + "no_config_manager": None, + "empty_config": _Cfg({}), + "plugin_id_config": _Cfg(_PLUGIN_ID_CONFIG), + "legacy_scoreboard_config": _Cfg(_LEGACY_SCOREBOARD_CONFIG), + "config_is_none": _Cfg(None), + "config_is_not_a_dict": _Cfg("x"), + "config_manager_without_config": _NoConfigAttr(), +} + +SPORT_KEYS = [None, "", "nfl", "nba", "mlb", "nhl", "soccer", "ncaa_fb", + "ncaa_baseball", "ncaam_basketball", "milb", + "football-scoreboard", "curling"] + + +def _fixed(max_age, memory_ttl, **extra): + return {"max_age": max_age, "memory_ttl": memory_ttl, + "force_refresh": False, **extra} + + +DEFAULT = _fixed(300, 600) +FIXED = { + "weather_current": _fixed(300, 600), + "stocks": _fixed(600, 1200, market_hours_only=True), + "crypto": _fixed(300, 600), + "sports_recent": _fixed(1800, 3600), + "sports_upcoming": _fixed(10800, 21600), + "sports_schedules": _fixed(86400, 172800), + "leaderboard": _fixed(604800, 1209600), + "news": _fixed(3600, 7200), + "odds": _fixed(1800, 3600), + "odds_live": _fixed(120, 240), + "team_info": _fixed(604800, 1209600), + "logos": _fixed(2592000, 5184000), + "default": DEFAULT, +} + + +def _expected(data_type, sport_key): + if data_type in ("live_scores", "sports_live"): + if sport_key: + interval = 60 + else: + interval = 15 if data_type == "live_scores" else 30 + return {"max_age": interval, "memory_ttl": interval * 2, + "force_refresh": True} + return FIXED.get(data_type, DEFAULT) + + +@pytest.mark.parametrize("cm_name", sorted(CONFIG_MANAGERS)) +def test_live_interval_is_60_for_every_sport(cm_name): + strategy = CacheStrategy(config_manager=CONFIG_MANAGERS[cm_name]) + for sport_key in SPORT_KEYS: + assert strategy.get_sport_live_interval(sport_key) == 60, sport_key + + +@pytest.mark.parametrize("cm_name", sorted(CONFIG_MANAGERS)) +def test_strategy_table_for_every_data_type_and_sport(cm_name): + strategy = CacheStrategy(config_manager=CONFIG_MANAGERS[cm_name]) + data_types = ["live_scores", "sports_live", *FIXED, "unknown", ""] + for data_type in data_types: + for sport_key in SPORT_KEYS: + got = strategy.get_cache_strategy(data_type, sport_key) + assert got == _expected(data_type, sport_key), (data_type, sport_key) + + +@pytest.mark.parametrize("key", [ + "soccer_live", "soccer_current", "soccer_scoreboard", "SOCCER_LIVE", + "nfl_live", "live", "hockey_current", "nba_live_scores", +]) +def test_live_keys_including_soccer_are_sports_live(key): + assert CacheStrategy().get_data_type_from_key(key) == "sports_live" + + +@pytest.mark.parametrize("key,data_type", [ + ("odds_soccer_live", "odds_live"), + ("odds_x", "odds"), + ("weather", "weather_current"), + ("crypto_stock", "crypto"), + ("stock", "stocks"), + ("news_soccer", "news"), + ("soccer_schedule", "sports_schedules"), + ("soccer_recent", "sports_recent"), + ("soccer_upcoming", "sports_upcoming"), + ("soccer_logo", "team_info"), + ("soccer", "default"), + ("", "default"), +]) +def test_non_live_keys_keep_their_data_type(key, data_type): + assert CacheStrategy().get_data_type_from_key(key) == data_type diff --git a/test/test_config_durable_writes.py b/test/test_config_durable_writes.py new file mode 100644 index 00000000..041d9884 --- /dev/null +++ b/test/test_config_durable_writes.py @@ -0,0 +1,347 @@ +""" +Every write of config.json / config_secrets.json goes through one durable +writer, atomic_write_text() in src/config_manager_atomic.py: temp file in the +same directory, fsync, rename, directory fsync. + +These pin what that buys on a device that loses power mid-save (the old file +survives intact), what it costs the SD card (the unchanged secrets file isn't +rewritten, backup rotation doesn't open every backup), and that the backups +it keeps are still the five config.json.backup. files anything +restoring from them expects. +""" + +import json +import os +import re +import stat +import threading +from pathlib import Path + +import pytest + +import src.config_manager_atomic as atomic_module +from src.config_manager import ConfigManager +from src.config_manager_atomic import AtomicConfigManager, SaveResultStatus, atomic_write_text +from src.exceptions import ConfigError + +ORIGINAL = {"timezone": "America/Chicago", "display": {"hardware": {"rows": 32}}} + +POSIX_ONLY = pytest.mark.skipif(os.name == 'nt', reason="POSIX file modes and directory fsync") + + +def make_manager(tmp_path, secrets=None): + config_file = tmp_path / "config.json" + config_file.write_text(json.dumps(ORIGINAL, indent=4)) + secrets_file = tmp_path / "config_secrets.json" + if secrets is not None: + secrets_file.write_text(json.dumps(secrets, indent=4)) + manager = ConfigManager(config_path=str(config_file), secrets_path=str(secrets_file)) + manager.template_path = str(tmp_path / "no-template.json") + return manager + + +def temp_leftovers(directory): + return [p.name for p in Path(directory).iterdir() if '.tmp.' in p.name] + + +class TestPowerCutLeavesTheOldConfig: + """A save that dies before the rename must leave config.json exactly as it was.""" + + def test_save_config_that_fails_midway_keeps_the_old_file(self, tmp_path): + # json.dump streams: an unencodable value deep in the dict used to + # leave config.json truncated at the point the encoder gave up. + manager = make_manager(tmp_path) + before = (tmp_path / "config.json").read_bytes() + + with pytest.raises(ConfigError): + manager.save_config({"timezone": "UTC", "zzz": object()}) + + assert (tmp_path / "config.json").read_bytes() == before + assert temp_leftovers(tmp_path) == [] + + def test_save_config_that_dies_before_the_rename_keeps_the_old_file(self, tmp_path, monkeypatch): + manager = make_manager(tmp_path) + before = (tmp_path / "config.json").read_bytes() + + def power_cut(*args, **kwargs): + raise OSError("power lost") + + monkeypatch.setattr(atomic_module.os, "replace", power_cut) + + with pytest.raises(ConfigError): + manager.save_config({"timezone": "UTC"}) + + assert (tmp_path / "config.json").read_bytes() == before + assert temp_leftovers(tmp_path) == [] + + def test_save_raw_file_content_that_dies_before_the_rename_keeps_the_old_file(self, tmp_path, monkeypatch): + manager = make_manager(tmp_path) + before = (tmp_path / "config.json").read_bytes() + + def power_cut(*args, **kwargs): + raise OSError("power lost") + + monkeypatch.setattr(atomic_module.os, "replace", power_cut) + + with pytest.raises(ConfigError): + manager.save_raw_file_content("main", {"timezone": "UTC"}) + + assert (tmp_path / "config.json").read_bytes() == before + assert temp_leftovers(tmp_path) == [] + + def test_save_config_atomic_that_dies_before_the_rename_keeps_the_old_file(self, tmp_path, monkeypatch): + manager = make_manager(tmp_path) + before = json.loads((tmp_path / "config.json").read_text()) + + real_replace = os.replace + + def power_cut(src, dst, *args, **kwargs): + if Path(dst).name == "config.json": + raise OSError("power lost") + return real_replace(src, dst, *args, **kwargs) + + monkeypatch.setattr(atomic_module.os, "replace", power_cut) + + result = manager.save_config_atomic({"timezone": "UTC"}, create_backup=False) + + assert result.status == SaveResultStatus.FAILED + assert json.loads((tmp_path / "config.json").read_text()) == before + assert temp_leftovers(tmp_path) == [] + + +class TestTheDataIsOnDiskBeforeTheRename: + def test_the_temp_file_is_fsynced_before_it_replaces_the_config(self, tmp_path, monkeypatch): + events = [] + real_fsync, real_replace = os.fsync, os.replace + + def fsync(fd): + events.append("fsync") + return real_fsync(fd) + + def replace(src, dst, *args, **kwargs): + events.append("replace") + return real_replace(src, dst, *args, **kwargs) + + monkeypatch.setattr(atomic_module.os, "fsync", fsync) + monkeypatch.setattr(atomic_module.os, "replace", replace) + + make_manager(tmp_path).save_config({"timezone": "UTC"}) + + assert "replace" in events + assert "fsync" in events[:events.index("replace")] + assert json.loads((tmp_path / "config.json").read_text()) == {"timezone": "UTC"} + + @POSIX_ONLY + def test_the_directory_is_fsynced_after_the_rename(self, tmp_path, monkeypatch): + events = [] + real_fsync, real_replace, real_open = os.fsync, os.replace, os.open + directory_fds = set() + + def open_(path, flags, *args, **kwargs): + fd = real_open(path, flags, *args, **kwargs) + if Path(path) == tmp_path: + directory_fds.add(fd) + return fd + + def fsync(fd): + events.append("dir-fsync" if fd in directory_fds else "fsync") + return real_fsync(fd) + + def replace(src, dst, *args, **kwargs): + events.append("replace") + return real_replace(src, dst, *args, **kwargs) + + monkeypatch.setattr(atomic_module.os, "open", open_) + monkeypatch.setattr(atomic_module.os, "fsync", fsync) + monkeypatch.setattr(atomic_module.os, "replace", replace) + + atomic_write_text(tmp_path / "config.json", "{}") + + assert events == ["fsync", "replace", "dir-fsync"] + + +class TestPermissionsSurviveTheRename: + @POSIX_ONLY + def test_config_and_secrets_get_their_shared_modes_before_the_rename(self, tmp_path, monkeypatch): + # mkstemp creates 0600. If the chmod came after the rename, the other + # service could open the new file in between and be refused. + seen = {} + real_replace = os.replace + + def replace(src, dst, *args, **kwargs): + seen[Path(dst).name] = stat.S_IMODE(os.stat(src).st_mode) + return real_replace(src, dst, *args, **kwargs) + + monkeypatch.setattr(atomic_module.os, "replace", replace) + + manager = make_manager(tmp_path, secrets={"weather": {"api_key": "k"}}) + manager.save_raw_file_content("main", {"timezone": "UTC"}) + manager.save_raw_file_content("secrets", {"weather": {"api_key": "new"}}) + + assert seen == {"config.json": 0o644, "config_secrets.json": 0o640} + assert stat.S_IMODE(os.stat(tmp_path / "config.json").st_mode) == 0o644 + assert stat.S_IMODE(os.stat(tmp_path / "config_secrets.json").st_mode) == 0o640 + + @POSIX_ONLY + def test_a_directory_named_secrets_does_not_lock_down_config_json(self, tmp_path): + # get_config_file_mode() looks for "secrets" anywhere in the string it + # is given; handed the full path, an install under e.g. + # ~/secrets-lab/LEDMatrix made config.json 0o640. + directory = tmp_path / "secrets-lab" + directory.mkdir() + + atomic_write_text(directory / "config.json", "{}") + + assert stat.S_IMODE(os.stat(directory / "config.json").st_mode) == 0o644 + + def test_a_root_save_hands_the_file_back_to_its_previous_owner(self, tmp_path, monkeypatch): + # The display service runs as root. A rename gives the file to the + # writer, so without this a root save would leave config.json owned by + # root and the web user could only ever replace it, never edit it. + target = tmp_path / "config.json" + target.write_text("{}") + previous = target.stat() + chowns = [] + + monkeypatch.setattr(atomic_module.os, "geteuid", lambda: 0, raising=False) + monkeypatch.setattr(atomic_module.os, "chown", + lambda path, uid, gid: chowns.append((Path(path), uid, gid)), + raising=False) + monkeypatch.setattr("src.common.permission_utils.get_shared_group_gid", lambda: None) + + atomic_write_text(target, '{"timezone": "UTC"}') + + assert len(chowns) == 1 + temp_path, uid, gid = chowns[0] + assert temp_path.parent == tmp_path and temp_path.name.startswith(".config.json.tmp.") + assert (uid, gid) == (previous.st_uid, previous.st_gid) + + def test_a_non_root_save_never_tries_to_chown(self, tmp_path, monkeypatch): + target = tmp_path / "config.json" + target.write_text("{}") + chowns = [] + + monkeypatch.setattr(atomic_module.os, "geteuid", lambda: 1000, raising=False) + monkeypatch.setattr(atomic_module.os, "chown", + lambda *args: chowns.append(args), raising=False) + + atomic_write_text(target, '{"timezone": "UTC"}') + + assert chowns == [] + + +@pytest.mark.skipif(os.name != 'nt', reason="only Windows refuses to rename over an open file") +class TestWindowsReaderHoldingTheFile: + def test_a_save_waits_out_a_reader_instead_of_failing(self, tmp_path): + target = tmp_path / "config.json" + target.write_text("{}") + reader = open(target) + closer = threading.Timer(0.15, reader.close) + closer.start() + try: + atomic_write_text(target, '{"timezone": "UTC"}') + finally: + closer.join() + reader.close() + assert json.loads(target.read_text()) == {"timezone": "UTC"} + + +class TestSecretsAreOnlyRewrittenWhenTheyChange: + @pytest.fixture + def writes(self, monkeypatch): + written = [] + real = atomic_module.atomic_write_text + + def spy(path, text, mode=None): + written.append(Path(path).name) + return real(path, text, mode) + + monkeypatch.setattr(atomic_module, "atomic_write_text", spy) + return written + + def test_config_manager_save_leaves_the_secrets_file_alone(self, tmp_path, writes): + secrets = {"weather": {"api_key": "k"}} + manager = make_manager(tmp_path, secrets=secrets) + + config = manager.load_config() + config["timezone"] = "UTC" + result = manager.save_config_atomic(config) + + assert result.status == SaveResultStatus.SUCCESS + assert writes == ["config.json"] + assert json.loads((tmp_path / "config_secrets.json").read_text()) == secrets + on_disk = json.loads((tmp_path / "config.json").read_text()) + assert on_disk["timezone"] == "UTC" and "api_key" not in on_disk.get("weather", {}) + + def test_identical_secrets_are_not_rewritten(self, tmp_path, writes): + secrets = {"weather": {"api_key": "k"}} + make_manager(tmp_path, secrets=secrets) + manager = AtomicConfigManager(str(tmp_path / "config.json"), + str(tmp_path / "config_secrets.json")) + + manager.save_config_atomic({"timezone": "UTC"}, new_secrets=dict(secrets)) + + assert writes == ["config.json"] + + def test_changed_secrets_are_written(self, tmp_path, writes): + make_manager(tmp_path, secrets={"weather": {"api_key": "old"}}) + manager = AtomicConfigManager(str(tmp_path / "config.json"), + str(tmp_path / "config_secrets.json")) + + manager.save_config_atomic({"timezone": "UTC"}, new_secrets={"weather": {"api_key": "new"}}) + + assert writes == ["config.json", "config_secrets.json"] + assert json.loads((tmp_path / "config_secrets.json").read_text()) == {"weather": {"api_key": "new"}} + + +class TestBackupRotation: + VERSION = re.compile(r"^\d{8}_\d{6}_\d{6}(-\d+)?$") + + def test_five_newest_backups_are_kept_under_the_same_names(self, tmp_path): + manager = make_manager(tmp_path, secrets={"weather": {"api_key": "k"}}) + backup_dir = tmp_path / "backups" + backup_dir.mkdir() + for version in ("20240101_120000", "20240102_120000"): + (backup_dir / f"config.json.backup.{version}").write_text("{}") + (backup_dir / f"config_secrets.json.backup.{version}").write_text("{}") + + created = [] + for i in range(6): + result = manager.save_config_atomic({"timezone": f"tz{i}"}) + assert result.status == SaveResultStatus.SUCCESS + created.append(Path(result.backup_path).name) + + config_backups = sorted(p.name for p in backup_dir.glob("config.json.backup.*")) + secrets_backups = sorted(p.name for p in backup_dir.glob("config_secrets.json.backup.*")) + + assert config_backups == sorted(created[-5:]) + for name in config_backups: + assert self.VERSION.match(name[len("config.json.backup."):]) + assert secrets_backups == sorted( + n.replace("config.json.backup.", "config_secrets.json.backup.") for n in created[-5:] + ) + assert [b.path for b in manager.list_backups()] == [ + str(backup_dir / n) for n in reversed(created[-5:]) + ] + + def test_rotation_does_not_open_the_backups(self, tmp_path, monkeypatch): + manager = make_manager(tmp_path) + for _ in range(3): + manager.save_config_atomic({"timezone": "UTC"}) + + parsed = [] + monkeypatch.setattr(AtomicConfigManager, "_validate_config_file", + lambda self, path: parsed.append(path)) + + result = manager.save_config_atomic({"timezone": "UTC"}) + assert result.status == SaveResultStatus.SUCCESS + assert parsed == [] + + def test_a_rollback_still_restores_from_a_rotated_backup(self, tmp_path): + manager = make_manager(tmp_path) + for i in range(7): + manager.save_config_atomic({"timezone": f"tz{i}"}) + + assert manager.rollback_config() + # The newest backup was taken just before the last save. + assert json.loads((tmp_path / "config.json").read_text()) == {"timezone": "tz5"}