fix: make fsync best-effort so only mkstemp/replace failures fall back

This commit is contained in:
Matt Van Horn
2026-07-05 03:53:22 -07:00
parent e0549d6c24
commit 961b54aa83
2 changed files with 34 additions and 10 deletions
+15 -10
View File
@@ -137,7 +137,7 @@ class AtomicJsonStore:
with os.fdopen(fd, "w", encoding="utf-8") as f: with os.fdopen(fd, "w", encoding="utf-8") as f:
self._write_payload(payload, f) self._write_payload(payload, f)
f.flush() f.flush()
os.fsync(f.fileno()) self._best_effort_fsync(f.fileno())
os.replace(tmp_path, self.path) os.replace(tmp_path, self.path)
self._fsync_directory(parent) self._fsync_directory(parent)
except Exception: except Exception:
@@ -151,15 +151,20 @@ class AtomicJsonStore:
with open(self.path, "w", encoding="utf-8") as f: with open(self.path, "w", encoding="utf-8") as f:
self._write_payload(payload, f) self._write_payload(payload, f)
f.flush() f.flush()
try: self._best_effort_fsync(f.fileno())
os.fsync(f.fileno())
except OSError as exc: @staticmethod
# Tolerate fsync being unsupported on the underlying filesystem def _best_effort_fsync(fileno: int) -> None:
# (the same class of filesystem that forced this fallback), but # Tolerate fsync being unsupported on the underlying filesystem (for
# let genuine storage failures such as ENOSPC/EIO surface rather # example a network mount that returns EINVAL/ENOSYS), but let genuine
# than reporting a durable write that did not happen. # storage failures such as ENOSPC/EIO surface so a non-durable write is
if exc.errno not in _ATOMIC_UNSUPPORTED_ERRNOS: # never reported as success. An unsupported fsync must not by itself
raise # abandon the atomic rename path.
try:
os.fsync(fileno)
except OSError as exc:
if exc.errno not in _ATOMIC_UNSUPPORTED_ERRNOS:
raise
@staticmethod @staticmethod
def _write_payload(payload: dict[str, Any], f: Any) -> None: def _write_payload(payload: dict[str, Any], f: Any) -> None:
+19
View File
@@ -71,6 +71,25 @@ class StateStoreTests(unittest.TestCase):
self.assertEqual(ctx.exception.errno, 13) self.assertEqual(ctx.exception.errno, 13)
self.assertFalse(os.path.exists(path)) self.assertFalse(os.path.exists(path))
def test_unsupported_fsync_keeps_atomic_path(self):
# fsync being unsupported (EINVAL/ENOSYS) must not by itself trigger the
# direct-write fallback; the atomic temp-file + rename path still runs.
import errno as _errno
with tempfile.TemporaryDirectory() as tmp:
path = os.path.join(tmp, "queue.json")
store = AtomicJsonStore(path, kind="persistent_queue:queue")
with patch(
"state_store.os.fsync",
side_effect=OSError(_errno.EINVAL, "Invalid argument"),
):
with self.assertNoLogs("state_store", level="WARNING"):
store.save({"items": [{"key": "a"}]})
self.assertEqual(store.load()["items"], [{"key": "a"}])
self.assertEqual([], [name for name in os.listdir(tmp) if name.endswith(".tmp")])
def test_save_reraises_and_preserves_state_on_non_atomic_errno(self): def test_save_reraises_and_preserves_state_on_non_atomic_errno(self):
# A storage failure such as ENOSPC is not an "atomic unavailable" # A storage failure such as ENOSPC is not an "atomic unavailable"
# signal, so it must surface instead of falling back to a direct write # signal, so it must surface instead of falling back to a direct write