From c095699720f51f5873f7382917cabe7702afc4fe Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Thu, 23 Jul 2026 20:00:41 +0000 Subject: [PATCH] fix(core): stop atomic writes from colliding on a constant PID suffix atomic_write_json/atomic_write_text build their temp filename as "{path}.tmp.{os.getpid()}". os.getpid() is constant for the life of a process, so it only ever distinguishes concurrent writers that live in different OS processes. Odysseus runs as a single long-lived process per container, so two concurrent writers to the same path (e.g. two request handlers racing a settings save) always compute the identical temp path. Whichever finishes os.replace() first removes the shared tmp file out from under the other, which then raises FileNotFoundError on its own os.replace() instead of landing its write. Fix: derive the temp suffix from uuid4() instead of the PID, so every call gets a distinct temp path regardless of process/thread identity. routes/prefs_routes.py's _save() had an independent, hand-rolled copy of the exact same PID-suffix logic (not the shared core.atomic_io helper other routes already use, e.g. routes/auth_routes.py) with the same bug. Replaced it with a call to atomic_write_json. Fixes #5596 --- core/atomic_io.py | 12 +++++--- routes/prefs_routes.py | 10 ++----- tests/test_atomic_io.py | 50 ++++++++++++++++++++++++++++---- tests/test_prefs_atomic_write.py | 5 ++-- 4 files changed, 57 insertions(+), 20 deletions(-) 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"})