diff --git a/core/atomic_io.py b/core/atomic_io.py index 81c640d8a..40a51adbe 100644 --- a/core/atomic_io.py +++ b/core/atomic_io.py @@ -15,17 +15,21 @@ import json import os +import uuid from typing import Any, Optional def atomic_write_json(path: str, data: Any, *, indent: Optional[int] = None) -> None: """Atomically persist `data` as JSON at `path`. - The temp file uses the live PID as a suffix so two processes saving the - same file (e.g. unit tests) don't collide on the rename target. + The temp file uses a random suffix so two concurrent writers saving the + same file don't collide on the rename target. A PID suffix does not do + this: the PID is constant for the life of a process, so two writers on + the same path within one process (or one single-process container, where + the PID never changes at all) still race for the same temp file. """ os.makedirs(os.path.dirname(path) or ".", exist_ok=True) - tmp = f"{path}.tmp.{os.getpid()}" + tmp = f"{path}.tmp.{uuid.uuid4().hex}" with open(tmp, "w", encoding="utf-8") as f: json.dump(data, f, indent=indent) f.flush() @@ -37,7 +41,7 @@ def atomic_write_text(path: str, text: str) -> None: if not isinstance(text, str): raise TypeError("atomic_write_text expects a string") os.makedirs(os.path.dirname(path) or ".", exist_ok=True) - tmp = f"{path}.tmp.{os.getpid()}" + tmp = f"{path}.tmp.{uuid.uuid4().hex}" with open(tmp, "w", encoding="utf-8") as f: f.write(text) f.flush() diff --git a/routes/prefs_routes.py b/routes/prefs_routes.py index f2a778c2d..91a17419e 100644 --- a/routes/prefs_routes.py +++ b/routes/prefs_routes.py @@ -1,8 +1,8 @@ """User preferences API — per-user key/value store backed by a JSON file.""" import json -import os from typing import Optional from fastapi import APIRouter, Request +from core.atomic_io import atomic_write_json from src.auth_helpers import get_current_user from src.constants import USER_PREFS_FILE @@ -20,13 +20,7 @@ def _load(): def _save(prefs): - os.makedirs(os.path.dirname(PREFS_FILE) or ".", exist_ok=True) - tmp = f"{PREFS_FILE}.tmp.{os.getpid()}" - with open(tmp, "w", encoding="utf-8") as f: - json.dump(prefs, f, indent=2) - f.flush() - os.fsync(f.fileno()) - os.replace(tmp, PREFS_FILE) + atomic_write_json(PREFS_FILE, prefs, indent=2) def _load_for_user(user: Optional[str] = None) -> dict: diff --git a/tests/test_atomic_io.py b/tests/test_atomic_io.py index 9fadec20a..e08da9db8 100644 --- a/tests/test_atomic_io.py +++ b/tests/test_atomic_io.py @@ -1,17 +1,19 @@ """Tests for ``core.atomic_io`` durability and crash-safety behavior. ``core.atomic_io`` provides ``atomic_write_json`` and ``atomic_write_text``. -Both write to a sibling ``.tmp.`` file, ``fsync`` it, then ``os.replace`` -into place so a crash mid-write leaves the previous good copy untouched rather -than a truncated/empty file. +Both write to a sibling ``.tmp.`` file, ``fsync`` it, then +``os.replace`` into place so a crash mid-write leaves the previous good copy +untouched rather than a truncated/empty file. These tests cover the happy path (round-trip, indent, parent-dir creation, -full overwrite, no leftover tmp) and the two failure paths the implementation -guarantees: the target file is preserved when serialization fails before the -replace, and when ``os.replace`` itself fails. +full overwrite, no leftover tmp), the two failure paths the implementation +guarantees (the target file is preserved when serialization fails before the +replace, and when ``os.replace`` itself fails), and that two concurrent +writers to the same path don't collide on the same temp file. """ import importlib.util import json +import threading from pathlib import Path import pytest @@ -84,6 +86,42 @@ def test_atomic_write_json_leaves_no_tmp_file(tmp_path): assert _tmp_siblings(tmp_path, "data.json") == [] +def test_atomic_write_json_concurrent_writers_do_not_collide(tmp_path): + # Both writers run in this same process, so a PID-based tmp suffix is + # identical for both: whichever writer finishes first unlinks the tmp + # file (via os.replace) out from under the other, which then raises + # FileNotFoundError on its own os.replace instead of landing its write. + target = tmp_path / "settings.json" + orig_dump = json.dump + barrier = threading.Barrier(2) + errors = [] + + def slow_dump(obj, fp, **kwargs): + orig_dump(obj, fp, **kwargs) + fp.flush() + barrier.wait() + + def write(payload): + try: + atomic_write_json(str(target), payload) + except Exception as exc: # noqa: BLE001 - captured for the assertion below + errors.append(exc) + + json.dump = slow_dump + try: + t1 = threading.Thread(target=write, args=({"writer": "A"},)) + t2 = threading.Thread(target=write, args=({"writer": "B"},)) + t1.start() + t2.start() + t1.join() + t2.join() + finally: + json.dump = orig_dump + + assert errors == [] + assert json.loads(target.read_text(encoding="utf-8"))["writer"] in ("A", "B") + + # --------------------------------------------------------------------------- # atomic_write_json — failure path: target preserved on serialization error. # --------------------------------------------------------------------------- diff --git a/tests/test_prefs_atomic_write.py b/tests/test_prefs_atomic_write.py index d7eac3087..29c24ccde 100644 --- a/tests/test_prefs_atomic_write.py +++ b/tests/test_prefs_atomic_write.py @@ -1,11 +1,12 @@ import json import routes.prefs_routes as prefs_routes +from core import atomic_io def test_save_replaces_prefs_file_atomically(monkeypatch, tmp_path): calls = [] - real_replace = prefs_routes.os.replace + real_replace = atomic_io.os.replace def fake_replace(src, dst): calls.append((src, dst)) @@ -13,7 +14,7 @@ def fake_replace(src, dst): prefs_file = tmp_path / "data" / "user_prefs.json" monkeypatch.setattr(prefs_routes, "PREFS_FILE", str(prefs_file)) - monkeypatch.setattr(prefs_routes.os, "replace", fake_replace) + monkeypatch.setattr(atomic_io.os, "replace", fake_replace) prefs_routes._save({"theme": "dark"})