From c2c129db6102b88b65e9f8c296dad91e3ada0057 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 5 Jul 2026 03:42:07 -0700 Subject: [PATCH 1/8] fix: fall back to direct write when atomic state save hits EPERM on NFS --- app/state_store.py | 35 ++++++++++++++++++++++-- app/tests/test_state_store.py | 50 +++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/app/state_store.py b/app/state_store.py index cab711a..2548ad6 100644 --- a/app/state_store.py +++ b/app/state_store.py @@ -62,6 +62,7 @@ class AtomicJsonStore: self.path = path self.kind = kind self.schema_version = schema_version + self._direct_write_fallback_warned = False def _ensure_parent(self) -> None: parent = os.path.dirname(self.path) @@ -96,6 +97,13 @@ class AtomicJsonStore: def save(self, data: dict[str, Any]) -> None: self._ensure_parent() payload = self._build_payload(data) + try: + self._atomic_write(payload) + except OSError as exc: + self._warn_direct_write_fallback(exc) + self._direct_write(payload) + + def _atomic_write(self, payload: dict[str, Any]) -> None: parent = os.path.dirname(self.path) or "." fd, tmp_path = tempfile.mkstemp( prefix=f".{os.path.basename(self.path)}.", @@ -105,8 +113,7 @@ class AtomicJsonStore: ) try: with os.fdopen(fd, "w", encoding="utf-8") as f: - json.dump(payload, f, ensure_ascii=False, separators=(",", ":")) - f.write("\n") + self._write_payload(payload, f) f.flush() os.fsync(f.fileno()) os.replace(tmp_path, self.path) @@ -118,6 +125,30 @@ class AtomicJsonStore: pass raise + def _direct_write(self, payload: dict[str, Any]) -> None: + with open(self.path, "w", encoding="utf-8") as f: + self._write_payload(payload, f) + f.flush() + try: + os.fsync(f.fileno()) + except OSError: + pass + + @staticmethod + def _write_payload(payload: dict[str, Any], f: Any) -> None: + json.dump(payload, f, ensure_ascii=False, separators=(",", ":")) + f.write("\n") + + def _warn_direct_write_fallback(self, exc: OSError) -> None: + if self._direct_write_fallback_warned: + return + self._direct_write_fallback_warned = True + log.warning( + "Atomic state write failed for %s (%s); falling back to direct write", + self.path, + exc, + ) + def quarantine_invalid_file(self, exc: Exception) -> None: if not os.path.exists(self.path): return diff --git a/app/tests/test_state_store.py b/app/tests/test_state_store.py index fb71b08..759d89a 100644 --- a/app/tests/test_state_store.py +++ b/app/tests/test_state_store.py @@ -4,6 +4,7 @@ import os import tempfile import unittest from datetime import datetime +from unittest.mock import patch from state_store import AtomicJsonStore, from_json_compatible, to_json_compatible @@ -21,6 +22,55 @@ class StateStoreTests(unittest.TestCase): self.assertEqual(payload["schema_version"], 2) self.assertEqual(payload["items"][0]["info"]["title"], "hello") + def test_save_falls_back_to_direct_write_when_mkstemp_fails(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "queue.json") + store = AtomicJsonStore(path, kind="persistent_queue:queue") + + with self.assertLogs("state_store", level="WARNING") as logs: + with patch( + "state_store.tempfile.mkstemp", + side_effect=PermissionError(1, "Operation not permitted"), + ): + store.save({"items": [{"key": "a"}]}) + + self.assertTrue(os.path.exists(path)) + self.assertTrue(any(path in message for message in logs.output)) + payload = store.load() + self.assertEqual(payload["items"], [{"key": "a"}]) + + def test_save_falls_back_to_direct_write_when_replace_fails(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "queue.json") + store = AtomicJsonStore(path, kind="persistent_queue:queue") + + with patch( + "state_store.os.replace", + side_effect=PermissionError(1, "Operation not permitted"), + ): + store.save({"items": [{"key": "a"}]}) + + self.assertTrue(os.path.exists(path)) + payload = store.load() + self.assertEqual(payload["items"], [{"key": "a"}]) + self.assertEqual([], [name for name in os.listdir(tmp) if name.endswith(".tmp")]) + + def test_save_reraises_when_atomic_and_direct_write_fail(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "queue.json") + store = AtomicJsonStore(path, kind="persistent_queue:queue") + + with patch( + "state_store.tempfile.mkstemp", + side_effect=PermissionError(1, "Operation not permitted"), + ): + with patch("builtins.open", side_effect=PermissionError(13, "Permission denied")): + with self.assertRaises(PermissionError) as ctx: + store.save({"items": [{"key": "a"}]}) + + self.assertEqual(ctx.exception.errno, 13) + self.assertFalse(os.path.exists(path)) + def test_invalid_file_is_quarantined(self): with tempfile.TemporaryDirectory() as tmp: path = os.path.join(tmp, "queue.json") From f315b75bb2436cb200e3b47110c83a09754465ad Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 5 Jul 2026 03:44:57 -0700 Subject: [PATCH 2/8] fix: limit atomic-write fallback to atomic-unsupported errnos --- app/state_store.py | 22 ++++++++++++++++++++++ app/tests/test_state_store.py | 22 ++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/app/state_store.py b/app/state_store.py index 2548ad6..2d2b2b7 100644 --- a/app/state_store.py +++ b/app/state_store.py @@ -2,6 +2,7 @@ from __future__ import annotations import base64 import collections.abc +import errno import json import logging import os @@ -17,6 +18,25 @@ STATE_SCHEMA_VERSION = 2 _BYTES_MARKER = "__metube_bytes__" _DATETIME_MARKER = "__metube_datetime__" +# Errnos that signal the filesystem cannot support the temp-file + rename +# atomic-write strategy (for example an NFS-backed state dir returning EPERM on +# mkstemp). These are safe to fall back on because they mean the atomic +# mechanism is unavailable, not that the data write itself failed. Errors like +# ENOSPC/EIO are deliberately excluded so a genuine storage failure surfaces +# instead of silently truncating an existing good state file. +_ATOMIC_UNSUPPORTED_ERRNOS = frozenset( + e + for e in ( + errno.EPERM, + errno.EACCES, + errno.ENOSYS, + errno.EINVAL, + getattr(errno, "EOPNOTSUPP", None), + getattr(errno, "ENOTSUP", None), + ) + if e is not None +) + def to_json_compatible(value: Any) -> Any: if value is None or isinstance(value, (bool, int, float, str)): @@ -100,6 +120,8 @@ class AtomicJsonStore: try: self._atomic_write(payload) except OSError as exc: + if exc.errno not in _ATOMIC_UNSUPPORTED_ERRNOS: + raise self._warn_direct_write_fallback(exc) self._direct_write(payload) diff --git a/app/tests/test_state_store.py b/app/tests/test_state_store.py index 759d89a..089ee95 100644 --- a/app/tests/test_state_store.py +++ b/app/tests/test_state_store.py @@ -71,6 +71,28 @@ class StateStoreTests(unittest.TestCase): self.assertEqual(ctx.exception.errno, 13) self.assertFalse(os.path.exists(path)) + def test_save_reraises_and_preserves_state_on_non_atomic_errno(self): + # A storage failure such as ENOSPC is not an "atomic unavailable" + # signal, so it must surface instead of falling back to a direct write + # that would truncate the existing good state file. + import errno as _errno + + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "queue.json") + store = AtomicJsonStore(path, kind="persistent_queue:queue") + store.save({"items": [{"key": "good"}]}) + + with patch( + "state_store.tempfile.mkstemp", + side_effect=OSError(_errno.ENOSPC, "No space left on device"), + ): + with self.assertRaises(OSError) as ctx: + store.save({"items": [{"key": "new"}]}) + + self.assertEqual(ctx.exception.errno, _errno.ENOSPC) + # Existing state is untouched. + self.assertEqual(store.load()["items"], [{"key": "good"}]) + def test_invalid_file_is_quarantined(self): with tempfile.TemporaryDirectory() as tmp: path = os.path.join(tmp, "queue.json") From e0549d6c24518014c16296b3e54f4bd2e25af8e0 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 5 Jul 2026 03:49:15 -0700 Subject: [PATCH 3/8] fix: surface real storage errors from direct-write fsync fallback --- app/state_store.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/state_store.py b/app/state_store.py index 2d2b2b7..5de4b24 100644 --- a/app/state_store.py +++ b/app/state_store.py @@ -153,8 +153,13 @@ class AtomicJsonStore: f.flush() try: os.fsync(f.fileno()) - except OSError: - pass + except OSError as exc: + # Tolerate fsync being unsupported on the underlying filesystem + # (the same class of filesystem that forced this fallback), but + # let genuine storage failures such as ENOSPC/EIO surface rather + # than reporting a durable write that did not happen. + if exc.errno not in _ATOMIC_UNSUPPORTED_ERRNOS: + raise @staticmethod def _write_payload(payload: dict[str, Any], f: Any) -> None: From 961b54aa837f3903bfcff7ff7fbf148c3305d540 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 5 Jul 2026 03:53:22 -0700 Subject: [PATCH 4/8] fix: make fsync best-effort so only mkstemp/replace failures fall back --- app/state_store.py | 25 +++++++++++++++---------- app/tests/test_state_store.py | 19 +++++++++++++++++++ 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/app/state_store.py b/app/state_store.py index 5de4b24..11bb5bb 100644 --- a/app/state_store.py +++ b/app/state_store.py @@ -137,7 +137,7 @@ class AtomicJsonStore: with os.fdopen(fd, "w", encoding="utf-8") as f: self._write_payload(payload, f) f.flush() - os.fsync(f.fileno()) + self._best_effort_fsync(f.fileno()) os.replace(tmp_path, self.path) self._fsync_directory(parent) except Exception: @@ -151,15 +151,20 @@ class AtomicJsonStore: with open(self.path, "w", encoding="utf-8") as f: self._write_payload(payload, f) f.flush() - try: - os.fsync(f.fileno()) - except OSError as exc: - # Tolerate fsync being unsupported on the underlying filesystem - # (the same class of filesystem that forced this fallback), but - # let genuine storage failures such as ENOSPC/EIO surface rather - # than reporting a durable write that did not happen. - if exc.errno not in _ATOMIC_UNSUPPORTED_ERRNOS: - raise + self._best_effort_fsync(f.fileno()) + + @staticmethod + def _best_effort_fsync(fileno: int) -> None: + # Tolerate fsync being unsupported on the underlying filesystem (for + # example a network mount that returns EINVAL/ENOSYS), but let genuine + # storage failures such as ENOSPC/EIO surface so a non-durable write is + # never reported as success. An unsupported fsync must not by itself + # abandon the atomic rename path. + try: + os.fsync(fileno) + except OSError as exc: + if exc.errno not in _ATOMIC_UNSUPPORTED_ERRNOS: + raise @staticmethod def _write_payload(payload: dict[str, Any], f: Any) -> None: diff --git a/app/tests/test_state_store.py b/app/tests/test_state_store.py index 089ee95..0189808 100644 --- a/app/tests/test_state_store.py +++ b/app/tests/test_state_store.py @@ -71,6 +71,25 @@ class StateStoreTests(unittest.TestCase): self.assertEqual(ctx.exception.errno, 13) 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): # A storage failure such as ENOSPC is not an "atomic unavailable" # signal, so it must surface instead of falling back to a direct write From 49a46a7d1c953eb927b887fe0441ac990973894c Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 5 Jul 2026 03:57:19 -0700 Subject: [PATCH 5/8] fix: create fallback state file with owner-only 0600 permissions --- app/state_store.py | 6 +++++- app/tests/test_state_store.py | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/state_store.py b/app/state_store.py index 11bb5bb..c2a69f7 100644 --- a/app/state_store.py +++ b/app/state_store.py @@ -148,7 +148,11 @@ class AtomicJsonStore: raise def _direct_write(self, payload: dict[str, Any]) -> None: - with open(self.path, "w", encoding="utf-8") as f: + # Create with 0o600 so the fallback keeps the owner-only permissions the + # atomic path gets from mkstemp; state files can contain URLs and + # per-download option overrides that must not leak on shared mounts. + fd = os.open(self.path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "w", encoding="utf-8") as f: self._write_payload(payload, f) f.flush() self._best_effort_fsync(f.fileno()) diff --git a/app/tests/test_state_store.py b/app/tests/test_state_store.py index 0189808..9309f4f 100644 --- a/app/tests/test_state_store.py +++ b/app/tests/test_state_store.py @@ -36,6 +36,8 @@ class StateStoreTests(unittest.TestCase): self.assertTrue(os.path.exists(path)) self.assertTrue(any(path in message for message in logs.output)) + # Fallback keeps owner-only permissions, matching the atomic path. + self.assertEqual(os.stat(path).st_mode & 0o777, 0o600) payload = store.load() self.assertEqual(payload["items"], [{"key": "a"}]) @@ -64,7 +66,10 @@ class StateStoreTests(unittest.TestCase): "state_store.tempfile.mkstemp", side_effect=PermissionError(1, "Operation not permitted"), ): - with patch("builtins.open", side_effect=PermissionError(13, "Permission denied")): + with patch( + "state_store.os.open", + side_effect=PermissionError(13, "Permission denied"), + ): with self.assertRaises(PermissionError) as ctx: store.save({"items": [{"key": "a"}]}) From 96e88a355528fa05f0de1cd2f4719b5c4d634382 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 5 Jul 2026 04:00:59 -0700 Subject: [PATCH 6/8] fix: force 0600 on fallback state rewrites, not just creation --- app/state_store.py | 8 ++++++++ app/tests/test_state_store.py | 17 +++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/app/state_store.py b/app/state_store.py index c2a69f7..1958286 100644 --- a/app/state_store.py +++ b/app/state_store.py @@ -153,6 +153,14 @@ class AtomicJsonStore: # per-download option overrides that must not leak on shared mounts. fd = os.open(self.path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) with os.fdopen(fd, "w", encoding="utf-8") as f: + # The 0o600 mode above only applies when the file is created; force + # it on rewrites too so an existing, broadly-permissioned state file + # is tightened to match the atomic path. Best-effort because some + # network filesystems reject chmod, and that must not re-crash save. + try: + os.fchmod(f.fileno(), 0o600) + except OSError: + pass self._write_payload(payload, f) f.flush() self._best_effort_fsync(f.fileno()) diff --git a/app/tests/test_state_store.py b/app/tests/test_state_store.py index 9309f4f..82e3e30 100644 --- a/app/tests/test_state_store.py +++ b/app/tests/test_state_store.py @@ -41,6 +41,23 @@ class StateStoreTests(unittest.TestCase): payload = store.load() self.assertEqual(payload["items"], [{"key": "a"}]) + def test_fallback_tightens_permissions_on_existing_file(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "queue.json") + with open(path, "w", encoding="utf-8") as f: + f.write("{}") + os.chmod(path, 0o644) + + store = AtomicJsonStore(path, kind="persistent_queue:queue") + with patch( + "state_store.tempfile.mkstemp", + side_effect=PermissionError(1, "Operation not permitted"), + ): + store.save({"items": [{"key": "a"}]}) + + self.assertEqual(os.stat(path).st_mode & 0o777, 0o600) + self.assertEqual(store.load()["items"], [{"key": "a"}]) + def test_save_falls_back_to_direct_write_when_replace_fails(self): with tempfile.TemporaryDirectory() as tmp: path = os.path.join(tmp, "queue.json") From b00d4785eeab9cf3e250781d060c004059cc5682 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 5 Jul 2026 04:04:54 -0700 Subject: [PATCH 7/8] fix: serialize state before truncating in the direct-write fallback --- app/state_store.py | 14 +++++++++----- app/tests/test_state_store.py | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/app/state_store.py b/app/state_store.py index 1958286..57ead6c 100644 --- a/app/state_store.py +++ b/app/state_store.py @@ -126,6 +126,7 @@ class AtomicJsonStore: self._direct_write(payload) def _atomic_write(self, payload: dict[str, Any]) -> None: + text = self._serialize(payload) parent = os.path.dirname(self.path) or "." fd, tmp_path = tempfile.mkstemp( prefix=f".{os.path.basename(self.path)}.", @@ -135,7 +136,7 @@ class AtomicJsonStore: ) try: with os.fdopen(fd, "w", encoding="utf-8") as f: - self._write_payload(payload, f) + f.write(text) f.flush() self._best_effort_fsync(f.fileno()) os.replace(tmp_path, self.path) @@ -148,6 +149,10 @@ class AtomicJsonStore: raise def _direct_write(self, payload: dict[str, Any]) -> None: + # Serialize before truncating so a serialization failure never destroys + # the existing state file (the atomic path gets this for free via its + # temp file). + text = self._serialize(payload) # Create with 0o600 so the fallback keeps the owner-only permissions the # atomic path gets from mkstemp; state files can contain URLs and # per-download option overrides that must not leak on shared mounts. @@ -161,7 +166,7 @@ class AtomicJsonStore: os.fchmod(f.fileno(), 0o600) except OSError: pass - self._write_payload(payload, f) + f.write(text) f.flush() self._best_effort_fsync(f.fileno()) @@ -179,9 +184,8 @@ class AtomicJsonStore: raise @staticmethod - def _write_payload(payload: dict[str, Any], f: Any) -> None: - json.dump(payload, f, ensure_ascii=False, separators=(",", ":")) - f.write("\n") + def _serialize(payload: dict[str, Any]) -> str: + return json.dumps(payload, ensure_ascii=False, separators=(",", ":")) + "\n" def _warn_direct_write_fallback(self, exc: OSError) -> None: if self._direct_write_fallback_warned: diff --git a/app/tests/test_state_store.py b/app/tests/test_state_store.py index 82e3e30..15e0880 100644 --- a/app/tests/test_state_store.py +++ b/app/tests/test_state_store.py @@ -134,6 +134,23 @@ class StateStoreTests(unittest.TestCase): # Existing state is untouched. self.assertEqual(store.load()["items"], [{"key": "good"}]) + def test_serialization_failure_preserves_existing_state(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "queue.json") + store = AtomicJsonStore(path, kind="persistent_queue:queue") + store.save({"items": [{"key": "good"}]}) + + # Even on the fallback path, a non-serializable payload must raise + # before the existing good state file is touched. + with patch( + "state_store.tempfile.mkstemp", + side_effect=PermissionError(1, "Operation not permitted"), + ): + with self.assertRaises(TypeError): + store.save({"items": object()}) + + self.assertEqual(store.load()["items"], [{"key": "good"}]) + def test_invalid_file_is_quarantined(self): with tempfile.TemporaryDirectory() as tmp: path = os.path.join(tmp, "queue.json") From 54463baf0ea8d27dcef1f6be18f42346d06014f8 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Sun, 5 Jul 2026 04:11:12 -0700 Subject: [PATCH 8/8] fix: fsync parent dir after direct-write fallback for durability parity --- app/state_store.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/state_store.py b/app/state_store.py index 57ead6c..940a882 100644 --- a/app/state_store.py +++ b/app/state_store.py @@ -169,6 +169,8 @@ class AtomicJsonStore: f.write(text) f.flush() self._best_effort_fsync(f.fileno()) + # Make the new directory entry durable too, matching the atomic path. + self._fsync_directory(os.path.dirname(self.path) or ".") @staticmethod def _best_effort_fsync(fileno: int) -> None: