From 203c68f97e5cafdd6a2b48df82c2472ce2521253 Mon Sep 17 00:00:00 2001
From: Tarun Teja Kalagara <taruntejakalagara@gmail.com>
Date: Fri, 29 May 2026 00:52:35 -0400
Subject: [PATCH] [tarun] fix: fsync tempfile and dir in atomic_write_json

---
 backend/config/json_store.py          | 27 +++++++++++++
 backend/tests/test_disk_resilience.py | 56 +++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/backend/config/json_store.py b/backend/config/json_store.py
index 46984696..2c0fd71e 100644
--- a/backend/config/json_store.py
+++ b/backend/config/json_store.py
@@ -32,10 +32,19 @@ def atomic_write_json(path: str, payload, *, indent: int = 2) -> None:
         try:
             with os.fdopen(fd, "w", encoding="utf-8") as f:
                 json.dump(payload, f, indent=indent)
+                # os.replace is atomic for the filename, but the file's data
+                # may still be sitting in the page cache when the rename
+                # commits. A power loss between rename and the kernel's next
+                # writeback can leave a zero-length or torn file even though
+                # the rename "succeeded". fsync before the rename is what
+                # actually makes this crash-safe.
+                f.flush()
+                os.fsync(f.fileno())
             # Windows: Defender can briefly hold the destination open; a couple of retries covers every real case.
             for attempt in range(3):
                 try:
                     os.replace(tmp, path)
+                    _fsync_dir(directory)
                     return
                 except PermissionError:
                     if attempt == 2:
@@ -49,6 +58,24 @@ def atomic_write_json(path: str, payload, *, indent: int = 2) -> None:
             raise
 
 
+def _fsync_dir(directory: str) -> None:
+    # Best-effort fsync of the parent dir so the rename itself sticks
+    # across a crash. POSIX needs this (ext4/xfs/btrfs); Windows doesn't
+    # let you open a directory for fsync, so we just skip there. Failing
+    # here is non-fatal: the file content is already fsync'd above and
+    # the rename has happened in memory.
+    try:
+        dir_fd = os.open(directory, os.O_RDONLY)
+    except OSError:
+        return
+    try:
+        os.fsync(dir_fd)
+    except OSError:
+        pass
+    finally:
+        os.close(dir_fd)
+
+
 def read_json_or_none(path: str) -> dict | None:
     """Parse `path`; return None (and log) on a missing/garbled file rather than
     raising. Schema validation is the caller's job, kept separate so a real
diff --git a/backend/tests/test_disk_resilience.py b/backend/tests/test_disk_resilience.py
index 9b37cbae..388ebcbc 100644
--- a/backend/tests/test_disk_resilience.py
+++ b/backend/tests/test_disk_resilience.py
@@ -49,6 +49,62 @@ def test_atomic_write_preserves_existing_when_new_write_fails(tmp_path):
     assert read_json_or_none(p) == {"good": 1}
 
 
+def test_atomic_write_fsyncs_file_before_rename(tmp_path, monkeypatch):
+    # Without fsync before os.replace, we only get filename-atomicity, not
+    # data durability. Catch the regression by counting fsync calls before
+    # the rename happens.
+    from backend.config import json_store
+
+    fsync_count_at_replace = []
+    fsync_calls = []
+    real_fsync = os.fsync
+    real_replace = os.replace
+
+    def tracking_fsync(fd):
+        fsync_calls.append(fd)
+        return real_fsync(fd)
+
+    def tracking_replace(src, dst):
+        fsync_count_at_replace.append(len(fsync_calls))
+        return real_replace(src, dst)
+
+    monkeypatch.setattr(json_store.os, "fsync", tracking_fsync)
+    monkeypatch.setattr(json_store.os, "replace", tracking_replace)
+    atomic_write_json(str(tmp_path / "x.json"), {"k": "v"})
+
+    assert fsync_count_at_replace == [1], (
+        f"expected fsync on the tempfile before os.replace, saw "
+        f"{fsync_count_at_replace[0] if fsync_count_at_replace else 0} fsync calls"
+    )
+
+
+def test_atomic_write_fsyncs_directory_after_rename(tmp_path, monkeypatch):
+    # POSIX only: the rename is only durable once the parent dir is fsync'd.
+    if not hasattr(os, "O_RDONLY"):
+        pytest.skip("directory fsync not applicable on this platform")
+    from backend.config import json_store
+
+    fsync_targets = []
+    real_fsync = os.fsync
+
+    def tracking_fsync(fd):
+        try:
+            st = os.fstat(fd)
+            fsync_targets.append("dir" if (st.st_mode & 0o170000) == 0o040000 else "file")
+        except OSError:
+            fsync_targets.append("unknown")
+        return real_fsync(fd)
+
+    monkeypatch.setattr(json_store.os, "fsync", tracking_fsync)
+    atomic_write_json(str(tmp_path / "x.json"), {"k": "v"})
+
+    assert "file" in fsync_targets, "expected fsync on the data file"
+    assert "dir" in fsync_targets, "expected fsync on the parent directory"
+
+
+# ---------------- read_json_or_none ----------------
+
+
 # ---------------- read_json_or_none ----------------
 
 def test_read_missing_returns_none(tmp_path):