Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions core/activity.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
10 changes: 8 additions & 2 deletions core/file_operations.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 '.'
Expand All @@ -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:
Expand Down
30 changes: 30 additions & 0 deletions tests/test_activity_feed.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
31 changes: 31 additions & 0 deletions tests/test_core_activity.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
7 changes: 6 additions & 1 deletion web/services/operation_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand Down
15 changes: 11 additions & 4 deletions web/services/settings_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,18 +97,25 @@ 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)
except OSError:
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]:
Expand Down
Loading