diff --git a/core/activity.py b/core/activity.py index e611febd..6dfec704 100644 --- a/core/activity.py +++ b/core/activity.py @@ -124,8 +124,18 @@ class FileActivity: run_id: Optional[str] = None run_source: str = "legacy" # "scheduled" | "web" | "cli" | "maintenance" | "legacy" - def to_dict(self) -> dict: - fmt = get_time_format() + def to_dict(self, time_format: Optional[str] = None) -> dict: + """Serialize for the dashboard/API. + + Args: + time_format: "12h" or "24h". When None, read once from settings. + When serializing a LIST, the caller should read the format once + and pass it to every entry. Calling get_time_format() per entry + re-reads the settings file each time, so a concurrent settings + write can make one entry fall back to the "24h" default and drop + its AM/PM suffix while its siblings keep theirs. + """ + fmt = time_format or get_time_format() if fmt == "12h": time_display = self.timestamp.strftime("%-I:%M:%S %p") else: diff --git a/core/file_operations.py b/core/file_operations.py index 8cc3a633..ecd2c59f 100644 --- a/core/file_operations.py +++ b/core/file_operations.py @@ -49,16 +49,20 @@ ARTICLE_SUFFIX_RE = re.compile(r'^(.*?),\s+(The|A|An)(\s*\(\d{4}\))?\s*$', re.IGNORECASE) -def save_json_atomically(filepath: str, data, label: str = "data") -> None: +def save_json_atomically(filepath: str, data, label: str = "data") -> bool: """Save JSON data to file atomically (write-to-temp-then-rename). Creates a temp file in the same directory, writes data, then atomically - replaces the target file. This prevents corruption from interrupted writes. + replaces the target file. This prevents corruption from interrupted writes + and ensures concurrent readers never observe a truncated/partial file. Args: filepath: Target file path. data: JSON-serializable data to write. label: Human-readable label for error messages. + + Returns: + True if the file was written and replaced successfully, False otherwise. """ try: dir_name = os.path.dirname(filepath) or '.' @@ -73,8 +77,10 @@ def save_json_atomically(filepath: str, data, label: str = "data") -> None: except OSError: pass raise + return True except IOError as e: logging.error(f"Could not save {label} file: {type(e).__name__}: {e}") + return False def is_subtitle_file(filepath: str) -> bool: diff --git a/tests/test_activity_feed.py b/tests/test_activity_feed.py index 45a3c24b..23b5da19 100644 --- a/tests/test_activity_feed.py +++ b/tests/test_activity_feed.py @@ -471,3 +471,33 @@ def test_summary_written_atomically(self, tmp_path): keyword = call_args[1] label = keyword.get("label") if "label" in keyword else (positional[2] if len(positional) > 2 else None) assert label == "run summaries", f"Expected label 'run summaries', got {label!r}" + + +# ============================================================================ +# save_json_atomically success/failure reporting +# ============================================================================ + +class TestSaveJsonAtomicallyReturn: + """save_json_atomically() must report success so callers (e.g. the settings + writer) can preserve their bool contract.""" + + def test_returns_true_on_success(self, tmp_path): + from core.file_operations import save_json_atomically + + target = tmp_path / "out.json" + ok = save_json_atomically(str(target), {"a": 1}, label="test") + + assert ok is True + assert json.loads(target.read_text()) == {"a": 1} + + def test_returns_false_on_write_failure(self, tmp_path): + from core.file_operations import save_json_atomically + + target = tmp_path / "out.json" + # Simulate a disk/IO failure during the atomic replace. + with patch('core.file_operations.os.replace', side_effect=IOError("disk full")): + ok = save_json_atomically(str(target), {"a": 1}, label="test") + + assert ok is False + # Original target must not be left in a partial state (temp is cleaned up). + assert not target.exists() diff --git a/tests/test_core_activity.py b/tests/test_core_activity.py index e52a3bd2..21f5ae06 100644 --- a/tests/test_core_activity.py +++ b/tests/test_core_activity.py @@ -70,6 +70,37 @@ def test_to_dict_12h_format(self): assert "PM" in d['time_display'] + def test_to_dict_explicit_time_format_skips_settings_read(self): + """Passing time_format must override settings and not read them. + + Guards the render-consistency fix: when serializing a list, the caller + reads the format once and passes it, so a concurrent settings write + can't make one entry fall back to the 24h default mid-render. + """ + fa = FileActivity( + timestamp=datetime(2026, 1, 15, 14, 30, 5), + action="Cached", + filename="movie.mkv", + ) + with patch('core.activity.get_time_format') as mock_get_fmt: + d = fa.to_dict(time_format='24h') + + mock_get_fmt.assert_not_called() + assert d['time_display'] == "14:30:05" + + @pytest.mark.skipif(sys.platform == 'win32', reason="%-I strftime is Linux-only") + def test_to_dict_explicit_12h_overrides_24h_settings(self): + """An explicit 12h arg wins even when settings say 24h.""" + fa = FileActivity( + timestamp=datetime(2026, 1, 15, 14, 30, 5), + action="Cached", + filename="movie.mkv", + ) + with patch('core.activity.get_time_format', return_value='24h'): + d = fa.to_dict(time_format='12h') + + assert "PM" in d['time_display'] + def test_zero_size_dash_display(self): with patch('core.activity.get_time_format', return_value='24h'): fa = FileActivity( diff --git a/web/services/operation_runner.py b/web/services/operation_runner.py index 6cace8ba..217426a3 100644 --- a/web/services/operation_runner.py +++ b/web/services/operation_runner.py @@ -689,7 +689,12 @@ def recent_activity(self) -> List[dict]: Reloads from disk to include entries written by MaintenanceRunner. """ activities = load_activity() - return [a.to_dict() for a in activities] + # Read the time format ONCE for the whole render. Calling get_time_format() + # inside each to_dict() re-reads the settings file per entry, so a concurrent + # settings write could make one row fall back to the 24h default and drop its + # AM/PM while its siblings keep theirs. + time_format = get_time_format() + return [a.to_dict(time_format=time_format) for a in activities] def _parse_size(self, size_str: str) -> int: """Parse a size string like '1.5 GB' into bytes.""" diff --git a/web/services/settings_service.py b/web/services/settings_service.py index dbe9c655..1f1107bc 100644 --- a/web/services/settings_service.py +++ b/web/services/settings_service.py @@ -97,10 +97,17 @@ def _load_raw(self) -> Dict[str, Any]: return {} def _save_raw(self, settings: Dict[str, Any]) -> bool: - """Save raw settings to file""" + """Save raw settings to file atomically (write-temp-then-replace). + + An in-place truncating write (open 'w') lets concurrent readers — e.g. + get_time_format() during a dashboard render — observe an empty/partial + file and silently fall back to defaults. The atomic replace prevents + that: readers always see either the old or the new complete file. + """ + from core.file_operations import save_json_atomically try: - with open(self.settings_file, 'w', encoding='utf-8') as f: - json.dump(settings, f, indent=2) + if not save_json_atomically(str(self.settings_file), settings, label="settings"): + return False # Restrict permissions — settings contain secrets (Plex token, password hashes) try: os.chmod(self.settings_file, 0o600) @@ -108,7 +115,7 @@ def _save_raw(self, settings: Dict[str, Any]) -> bool: pass # Non-fatal (Windows, Docker with different uid) self._cached_settings = None # Invalidate cache return True - except IOError: + except (IOError, OSError): return False def _sanitize_path(self, path: Optional[str]) -> Optional[str]: