From 269385c97c53de164b861fe987a3e2ccb5a3610a Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Wed, 23 Sep 2026 12:52:50 -0400 Subject: [PATCH 1/2] fix(config): write config.json through one durable atomic writer (#611) save_config() opened config.json with 'w' and streamed json.dump into it, so a power cut or an unencodable value left the file truncated. save_config_atomic() renamed a temp file into place but never fsynced it, rewrote the unchanged secrets file on every save, and re-parsed every backup to rotate them. save_raw_file_content() had its own third copy. All of them, plus rollback and config creation from the template, now go through atomic_write_text(): temp file in the same directory, fsync, final mode set before the rename, rename (retried on Windows while a reader holds the file), directory fsync. A root save copies the previous owner onto the new file so a rename by the display service no longer hands config.json to root; the shared-group fix-up is unchanged. The mode is chosen from the file name, so a "secrets" directory in the install path no longer makes config.json 0640. The secrets file is rewritten only when its content changes, and backup rotation works from filenames alone. Backups keep their names (config/backups/config.json.backup., paired secrets backup) and the five newest are kept, as before. Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 16 + src/config_manager.py | 84 ++---- src/config_manager_atomic.py | 465 +++++++++++++++-------------- test/test_config_durable_writes.py | 347 +++++++++++++++++++++ 4 files changed, 619 insertions(+), 293 deletions(-) create mode 100644 test/test_config_durable_writes.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bb8c7ac..36c80b86 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. + ## 3.5.0 New modules a plugin may import via `src.*` (floor on 3.5.0): 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_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"} From 9d024f24efd1f891282b34ef61b3913238f96e3c Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Wed, 23 Sep 2026 12:54:07 -0400 Subject: [PATCH 2/2] refactor(cache): remove the cache layer's duplicate cleanup and dead lookups (#613) * refactor(cache): collapse CacheStrategy's all-60 defaults table and twin soccer branch get_sport_live_interval() without a config manager looked the sport up in a table where every value was 60, with 60 as the fallback; it now returns 60. get_data_type_from_key() had an `if 'soccer'` branch returning the same 'sports_live' as its else. test_cache_strategy_intervals pins the returned strategy for every data type x sport key x config-manager shape; it passes unchanged on the old code. A 2,544-entry dump of every CacheStrategy method over a wider grid is identical before and after. Co-Authored-By: Claude Opus 5.5 * refactor(cache): drop CacheStrategy's `_scoreboard` config lookup get_sport_live_interval() and get_cache_strategy() read live/recent/ upcoming intervals from config[f"{sport}_scoreboard"]. Those sections belonged to the built-in scoreboards the plugin system replaced; plugin config is keyed by plugin id ("football-scoreboard"), so on a current config the lookup always fell through to the defaults (60 live, 1800 recent, 10800 upcoming), which are now returned directly. The one input where this differs: a config.json upgraded from the pre-plugin era that still carries e.g. an "nfl_scoreboard" section (no code removes them), queried with an explicit sport key. No caller in core or the plugin monorepo passes a sport key here -- get_with_auto_strategy only derives one for keys classed sports_live/live_scores, and its callers (odds managers, odds-ticker) use odds keys -- so the stale section was unreachable in practice. A dump of every CacheStrategy method over 2,544 inputs differs from the previous commit only in those 45 legacy-config entries; the test grid now includes that shape. Co-Authored-By: Claude Opus 5.5 * perf(cache): list cache files without holding the memory-tier lock CacheManager.list_cache_files() held the in-memory cache's lock while it listed and stat'd the whole cache directory -- 8,864 files on a real rig -- so every get()/set() from the display loop and plugins waited out the scan. The lock never protected the disk: DiskCache writes and deletes under their own lock, and a file vanishing between listdir and stat was already handled (logged and skipped). The body is unchanged apart from the dedent. Co-Authored-By: Claude Opus 5.5 * refactor(cache): delegate memory-tier cleanup and stats to MemoryCache CacheManager._cleanup_memory_cache() was a line-for-line copy of MemoryCache.cleanup(), and get_memory_cache_stats() a copy of MemoryCache.get_stats(), both reaching into the component's private _cache/_timestamps/_lock through "backward compatibility" aliases bound in __init__. So the component's own cleanup and stats only ever ran in tests, and the aliases went stale whenever the component was swapped (test_cache_ttl_honoured does). Both now delegate, and the aliases are gone: nothing in core, the tests, or the ledmatrix-plugins monorepo reads them. Behaviour is the same. Compared line by line, the two cleanups differ only in the sort key's fallback (0 vs 0.0, which orders identically), range+bounds check vs slice for the eviction, and the logger name on the DEBUG summary line (src.cache_manager -> src.cache.memory_cache). A differential run over 20,000 random memory states (str/None/garbage/ future timestamps, orphan keys, sizes 0-12, forced and throttled runs) gives identical removed counts, resulting dicts and last-cleanup times; the same harness catches each of three seeded mutations of MemoryCache.cleanup. The throttle clock also moves with it: CacheManager kept its own copy of last-cleanup, the component's is used now, and they started equal. Co-Authored-By: Claude Opus 5.5 * refactor(background): inline the sport cache key and drop the unused request queue get_sport_cache_key() constructed a whole CacheManager -- ConfigManager, config parse, cache-dir probing with test-file writes -- to return f"{sport}_{date}". It now builds the key itself in the same format as CacheManager.generate_sport_cache_key() (UTC date, %Y%m%d); tests check the two agree for explicit dates and, with a frozen clock at 03:30 UTC, for the default date. Median per call on Windows: ~0.6 ms -> ~2 us (alternating runs); on a Pi the old path also wrote a probe file per call. request_queue was a PriorityQueue nothing ever put into: requests go straight to the executor, so `priority` never did anything. The queue is gone; the `priority` parameter and FetchRequest field stay (every monorepo scoreboard passes priority=) and are documented as ignored, and get_statistics() keeps reporting queue_size, now a literal 0 as it always was in practice. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- src/background_data_service.py | 25 ++-- src/cache/cache_strategy.py | 78 ++++------- src/cache_manager.py | 179 ++++++++----------------- test/test_background_data_service.py | 68 ++++++++++ test/test_cache_manager_memory_tier.py | 118 ++++++++++++++++ test/test_cache_strategy_intervals.py | 136 +++++++++++++++++++ 6 files changed, 416 insertions(+), 188 deletions(-) create mode 100644 test/test_cache_manager_memory_tier.py create mode 100644 test/test_cache_strategy_intervals.py 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/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