Merge c095699720 into 20e7fc0164

core/atomic_io.py

@@ -15,17 +15,21 @@ from __future__ import annotations
 
 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()

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:

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.<pid>`` 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.<random>`` 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.
 # ---------------------------------------------------------------------------

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 test_save_replaces_prefs_file_atomically(monkeypatch, tmp_path):
 
     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"})