From d302c14ad266684e7c13aff8a6764680152bf375 Mon Sep 17 00:00:00 2001 From: Shay Palachy-Affek Date: Fri, 6 Mar 2026 16:11:05 +0200 Subject: [PATCH 1/7] Release v4.3.0 --- src/cachier/version.info | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cachier/version.info b/src/cachier/version.info index 6aba2b24..80895903 100644 --- a/src/cachier/version.info +++ b/src/cachier/version.info @@ -1 +1 @@ -4.2.0 +4.3.0 From 19cd05264720c8bba94f46a18b189eb0d903d228 Mon Sep 17 00:00:00 2001 From: Shay Palachy Date: Fri, 6 Mar 2026 22:28:17 +0200 Subject: [PATCH 2/7] Fix pickle cleanup rewrite race --- src/cachier/cores/pickle.py | 27 ++++++++++- tests/pickle_tests/test_pickle_core.py | 64 ++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 2 deletions(-) diff --git a/src/cachier/cores/pickle.py b/src/cachier/cores/pickle.py index e9395822..35f7c59c 100644 --- a/src/cachier/cores/pickle.py +++ b/src/cachier/cores/pickle.py @@ -9,6 +9,7 @@ import logging import os import pickle # for local caching +import tempfile import time from contextlib import suppress from datetime import datetime, timedelta @@ -71,6 +72,10 @@ def on_modified(self, event) -> None: """A Watchdog Event Handler method.""" # noqa: D401 self._check_calculation() + def on_moved(self, event) -> None: + """A Watchdog Event Handler method.""" # noqa: D401 + self._check_calculation() + def __init__( self, hash_func: Optional[HashFunc], @@ -181,9 +186,27 @@ def _save_cache( fpath += f"_{separate_file_key}" elif hash_str is not None: fpath += f"_{hash_str}" + parent_dir = os.path.dirname(fpath) with self.lock: - with portalocker.Lock(fpath, mode="wb") as cf: - pickle.dump(cache, cast(IO[bytes], cf), protocol=4) + if isinstance(cache, CacheEntry): + with portalocker.Lock(fpath, mode="wb") as cache_file: + pickle.dump(cache, cast(IO[bytes], cache_file), protocol=4) + else: + with portalocker.Lock(fpath, mode="ab"): + with tempfile.NamedTemporaryFile( + mode="wb", + dir=parent_dir, + delete=False, + ) as temp_file: + temp_path = temp_file.name + pickle.dump(cache, cast(IO[bytes], temp_file), protocol=4) + temp_file.flush() + os.fsync(temp_file.fileno()) + try: + os.replace(temp_path, fpath) + finally: + with suppress(FileNotFoundError): + os.remove(temp_path) # the same as check for separate_file, but changed for typing if isinstance(cache, dict): self._cache_dict = cache diff --git a/tests/pickle_tests/test_pickle_core.py b/tests/pickle_tests/test_pickle_core.py index 66e27c6b..5242f622 100644 --- a/tests/pickle_tests/test_pickle_core.py +++ b/tests/pickle_tests/test_pickle_core.py @@ -1053,6 +1053,70 @@ def mock_get_cache_dict(): assert result == "result" +@pytest.mark.pickle +def test_save_cache_keeps_existing_file_readable_during_write(tmp_path, monkeypatch): + """Test that cache rewrites do not expose a truncated file to plain readers.""" + core = _PickleCore( + hash_func=None, + cache_dir=tmp_path, + pickle_reload=False, + wait_for_calc_timeout=10, + separate_files=False, + ) + + def mock_func(): + pass + + core.set_func(mock_func) + + initial_cache = { + "key1": CacheEntry( + value="result-1", + time=datetime.now(), + stale=False, + _processing=False, + ) + } + updated_cache = { + **initial_cache, + "key2": CacheEntry( + value="result-2", + time=datetime.now(), + stale=False, + _processing=False, + ), + } + core._save_cache(initial_cache) + + dump_started = threading.Event() + allow_dump = threading.Event() + real_pickle_dump = pickle.dump + + def blocking_dump(obj, fh, protocol): + if obj is updated_cache: + dump_started.set() + assert allow_dump.wait(timeout=5) + return real_pickle_dump(obj, fh, protocol=protocol) + + monkeypatch.setattr("cachier.cores.pickle.pickle.dump", blocking_dump) + + writer = threading.Thread(target=core._save_cache, args=(updated_cache,), daemon=True) + writer.start() + + assert dump_started.wait(timeout=5) + with open(core.cache_fpath, "rb") as cache_file: + visible_cache = pickle.load(cache_file) + assert visible_cache == initial_cache + + allow_dump.set() + writer.join(timeout=5) + assert not writer.is_alive() + + with open(core.cache_fpath, "rb") as cache_file: + visible_cache = pickle.load(cache_file) + assert visible_cache == updated_cache + + @pytest.mark.pickle def test_wait_with_polling_calls_timeout_check_when_processing(tmp_path): """Test _wait_with_polling checks timeout while entry is processing.""" From 4d6b04f279a6bd1e454d7d0440721a18cdbf019f Mon Sep 17 00:00:00 2001 From: Shay Palachy Date: Fri, 6 Mar 2026 22:40:20 +0200 Subject: [PATCH 3/7] Fix shared pickle cache locking on Windows --- src/cachier/cores/pickle.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/cachier/cores/pickle.py b/src/cachier/cores/pickle.py index 35f7c59c..7657e09e 100644 --- a/src/cachier/cores/pickle.py +++ b/src/cachier/cores/pickle.py @@ -29,6 +29,8 @@ class _PickleCore(_BaseCore): """The pickle core class for cachier.""" + _SHARED_LOCK_SUFFIX = ".lock" + class CacheChangeHandler(PatternMatchingEventHandler): """Handles cache-file modification events.""" @@ -102,6 +104,10 @@ def cache_fpath(self) -> str: os.makedirs(self.cache_dir, exist_ok=True) return os.path.abspath(os.path.join(os.path.realpath(self.cache_dir), self.cache_fname)) + @property + def _shared_lock_fpath(self) -> str: + return f"{self.cache_fpath}{self._SHARED_LOCK_SUFFIX}" + @staticmethod def _convert_legacy_cache_entry( entry: Union[dict, CacheEntry], @@ -118,8 +124,8 @@ def _convert_legacy_cache_entry( def _load_cache_dict(self) -> Dict[str, CacheEntry]: try: - with portalocker.Lock(self.cache_fpath, mode="rb") as cf: - cache = pickle.load(cast(IO[bytes], cf)) + with portalocker.Lock(self._shared_lock_fpath, mode="a+b"), open(self.cache_fpath, "rb") as cache_file: + cache = pickle.load(cast(IO[bytes], cache_file)) self._cache_used_fpath = str(self.cache_fpath) except (FileNotFoundError, EOFError): cache = {} @@ -192,7 +198,7 @@ def _save_cache( with portalocker.Lock(fpath, mode="wb") as cache_file: pickle.dump(cache, cast(IO[bytes], cache_file), protocol=4) else: - with portalocker.Lock(fpath, mode="ab"): + with portalocker.Lock(self._shared_lock_fpath, mode="a+b"): with tempfile.NamedTemporaryFile( mode="wb", dir=parent_dir, From 7eedff37112a8bdbfb5aa3e57db5055fc0def439 Mon Sep 17 00:00:00 2001 From: Shay Palachy Date: Fri, 6 Mar 2026 23:12:23 +0200 Subject: [PATCH 4/7] Keep pickle lockfiles out of cache dirs --- src/cachier/cores/pickle.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/cachier/cores/pickle.py b/src/cachier/cores/pickle.py index 7657e09e..95371d2c 100644 --- a/src/cachier/cores/pickle.py +++ b/src/cachier/cores/pickle.py @@ -6,6 +6,7 @@ # Licensed under the MIT license: # http://www.opensource.org/licenses/MIT-license # Copyright (c) 2016, Shay Palachy +import hashlib import logging import os import pickle # for local caching @@ -106,7 +107,10 @@ def cache_fpath(self) -> str: @property def _shared_lock_fpath(self) -> str: - return f"{self.cache_fpath}{self._SHARED_LOCK_SUFFIX}" + lock_dir = os.path.join(tempfile.gettempdir(), "cachier-locks") + os.makedirs(lock_dir, exist_ok=True) + cache_hash = hashlib.sha256(self.cache_fpath.encode("utf-8")).hexdigest() + return os.path.join(lock_dir, f"{cache_hash}{self._SHARED_LOCK_SUFFIX}") @staticmethod def _convert_legacy_cache_entry( @@ -285,6 +289,7 @@ async def amark_entry_being_calculated(self, key: str) -> None: def mark_entry_not_calculated(self, key: str) -> None: if self.separate_files: self._mark_entry_not_calculated_separate_files(key) + return # pragma: no cover with self.lock: cache = self.get_cache_dict() # that's ok, we don't need an entry in that case From 1aef855696dd41b5d453164289521c9cc8777f52 Mon Sep 17 00:00:00 2001 From: Shay Palachy Date: Fri, 6 Mar 2026 23:14:51 +0200 Subject: [PATCH 5/7] Remove version bump from pickle bugfix PR --- src/cachier/version.info | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cachier/version.info b/src/cachier/version.info index 80895903..6aba2b24 100644 --- a/src/cachier/version.info +++ b/src/cachier/version.info @@ -1 +1 @@ -4.3.0 +4.2.0 From a844708b2e9c4ad700a5b271a2a2e63775ffecd2 Mon Sep 17 00:00:00 2001 From: Shay Palachy Date: Fri, 6 Mar 2026 23:53:53 +0200 Subject: [PATCH 6/7] Harden pickle temp-file cleanup and lock fallback --- src/cachier/cores/pickle.py | 38 +++++--- tests/pickle_tests/test_pickle_core.py | 124 +++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 14 deletions(-) diff --git a/src/cachier/cores/pickle.py b/src/cachier/cores/pickle.py index 95371d2c..52f304c2 100644 --- a/src/cachier/cores/pickle.py +++ b/src/cachier/cores/pickle.py @@ -107,10 +107,18 @@ def cache_fpath(self) -> str: @property def _shared_lock_fpath(self) -> str: - lock_dir = os.path.join(tempfile.gettempdir(), "cachier-locks") - os.makedirs(lock_dir, exist_ok=True) cache_hash = hashlib.sha256(self.cache_fpath.encode("utf-8")).hexdigest() - return os.path.join(lock_dir, f"{cache_hash}{self._SHARED_LOCK_SUFFIX}") + candidate_dirs = ( + os.path.join(tempfile.gettempdir(), "cachier-locks"), + os.path.join(os.path.dirname(self.cache_fpath), ".cachier-locks"), + ) + for lock_dir in candidate_dirs: + try: + os.makedirs(lock_dir, exist_ok=True) + return os.path.join(lock_dir, f"{cache_hash}{self._SHARED_LOCK_SUFFIX}") + except OSError: + continue + return os.path.join(os.path.dirname(self.cache_fpath), f".{cache_hash}{self._SHARED_LOCK_SUFFIX}") @staticmethod def _convert_legacy_cache_entry( @@ -203,20 +211,22 @@ def _save_cache( pickle.dump(cache, cast(IO[bytes], cache_file), protocol=4) else: with portalocker.Lock(self._shared_lock_fpath, mode="a+b"): - with tempfile.NamedTemporaryFile( - mode="wb", - dir=parent_dir, - delete=False, - ) as temp_file: - temp_path = temp_file.name - pickle.dump(cache, cast(IO[bytes], temp_file), protocol=4) - temp_file.flush() - os.fsync(temp_file.fileno()) + temp_path = "" try: + with tempfile.NamedTemporaryFile( + mode="wb", + dir=parent_dir, + delete=False, + ) as temp_file: + temp_path = temp_file.name + pickle.dump(cache, cast(IO[bytes], temp_file), protocol=4) + temp_file.flush() + os.fsync(temp_file.fileno()) os.replace(temp_path, fpath) finally: - with suppress(FileNotFoundError): - os.remove(temp_path) + if temp_path: + with suppress(FileNotFoundError): + os.remove(temp_path) # the same as check for separate_file, but changed for typing if isinstance(cache, dict): self._cache_dict = cache diff --git a/tests/pickle_tests/test_pickle_core.py b/tests/pickle_tests/test_pickle_core.py index 5242f622..0c51a099 100644 --- a/tests/pickle_tests/test_pickle_core.py +++ b/tests/pickle_tests/test_pickle_core.py @@ -709,6 +709,130 @@ def mock_func(): core._save_cache({"key": "value"}, separate_file_key="test_key") +@pytest.mark.pickle +def test_save_cache_removes_temp_file_when_fsync_fails(tmp_path): + """Test _save_cache removes the temp file when fsync fails.""" + core = _PickleCore( + hash_func=None, + cache_dir=tmp_path, + pickle_reload=False, + wait_for_calc_timeout=10, + separate_files=False, + ) + + def mock_func(): + pass + + core.set_func(mock_func) + + with patch("cachier.cores.pickle.os.fsync", side_effect=OSError("fsync failed")), pytest.raises( + OSError, match="fsync failed" + ): + core._save_cache( + {"key": CacheEntry(value="value", time=datetime.now(), stale=False, _processing=False)} + ) + + assert list(tmp_path.iterdir()) == [] + + +@pytest.mark.pickle +def test_save_cache_propagates_tempfile_creation_failure_without_cleanup_error(tmp_path): + """Test _save_cache handles temp-file creation failures before temp_path exists.""" + core = _PickleCore( + hash_func=None, + cache_dir=tmp_path, + pickle_reload=False, + wait_for_calc_timeout=10, + separate_files=False, + ) + + def mock_func(): + pass + + core.set_func(mock_func) + + with patch("cachier.cores.pickle.tempfile.NamedTemporaryFile", side_effect=OSError("tempfile failed")), patch( + "cachier.cores.pickle.os.replace" + ) as mock_replace, patch("cachier.cores.pickle.os.remove") as mock_remove, pytest.raises( + OSError, match="tempfile failed" + ): + core._save_cache( + {"key": CacheEntry(value="value", time=datetime.now(), stale=False, _processing=False)} + ) + + mock_replace.assert_not_called() + mock_remove.assert_not_called() + assert list(tmp_path.iterdir()) == [] + + +@pytest.mark.pickle +def test_shared_lock_fpath_falls_back_to_cache_dir_when_temp_dir_unwritable(tmp_path): + """Test _shared_lock_fpath falls back when the system temp dir is not writable.""" + core = _PickleCore( + hash_func=None, + cache_dir=tmp_path, + pickle_reload=False, + wait_for_calc_timeout=10, + separate_files=False, + ) + + def mock_func(): + pass + + core.set_func(mock_func) + + temp_lock_dir = os.path.join("/non-writable-temp", "cachier-locks") + fallback_lock_dir = os.path.join(core.cache_dir, ".cachier-locks") + + def mock_makedirs(path, exist_ok=False): + if path in (core.cache_dir, fallback_lock_dir): + return None + if path == temp_lock_dir: + raise PermissionError("temp dir not writable") + raise AssertionError(f"Unexpected os.makedirs path: {path}") + + with patch("cachier.cores.pickle.tempfile.gettempdir", return_value="/non-writable-temp"), patch( + "cachier.cores.pickle.os.makedirs", side_effect=mock_makedirs + ): + assert core._shared_lock_fpath == os.path.join( + fallback_lock_dir, + f"{hashlib.sha256(core.cache_fpath.encode('utf-8')).hexdigest()}{core._SHARED_LOCK_SUFFIX}", + ) + + +@pytest.mark.pickle +def test_shared_lock_fpath_uses_cache_dir_file_when_lock_dirs_unwritable(tmp_path): + """Test _shared_lock_fpath falls back to a lockfile in the cache dir.""" + core = _PickleCore( + hash_func=None, + cache_dir=tmp_path, + pickle_reload=False, + wait_for_calc_timeout=10, + separate_files=False, + ) + + def mock_func(): + pass + + core.set_func(mock_func) + + temp_lock_dir = os.path.join("/non-writable-temp", "cachier-locks") + fallback_lock_dir = os.path.join(core.cache_dir, ".cachier-locks") + cache_hash = hashlib.sha256(core.cache_fpath.encode("utf-8")).hexdigest() + + def mock_makedirs(path, exist_ok=False): + if path == core.cache_dir: + return None + if path in (temp_lock_dir, fallback_lock_dir): + raise PermissionError("lock dir not writable") + raise AssertionError(f"Unexpected os.makedirs path: {path}") + + with patch("cachier.cores.pickle.tempfile.gettempdir", return_value="/non-writable-temp"), patch( + "cachier.cores.pickle.os.makedirs", side_effect=mock_makedirs + ): + assert core._shared_lock_fpath == os.path.join(core.cache_dir, f".{cache_hash}{core._SHARED_LOCK_SUFFIX}") + + @pytest.mark.pickle def test_set_entry_should_not_store(tmp_path): """Test set_entry when value should not be stored.""" From 3cdd9408fc46b7999cea217255ab68f82932aab9 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 6 Mar 2026 21:54:13 +0000 Subject: [PATCH 7/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/pickle_tests/test_pickle_core.py | 32 +++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/pickle_tests/test_pickle_core.py b/tests/pickle_tests/test_pickle_core.py index 0c51a099..451363da 100644 --- a/tests/pickle_tests/test_pickle_core.py +++ b/tests/pickle_tests/test_pickle_core.py @@ -725,12 +725,11 @@ def mock_func(): core.set_func(mock_func) - with patch("cachier.cores.pickle.os.fsync", side_effect=OSError("fsync failed")), pytest.raises( - OSError, match="fsync failed" + with ( + patch("cachier.cores.pickle.os.fsync", side_effect=OSError("fsync failed")), + pytest.raises(OSError, match="fsync failed"), ): - core._save_cache( - {"key": CacheEntry(value="value", time=datetime.now(), stale=False, _processing=False)} - ) + core._save_cache({"key": CacheEntry(value="value", time=datetime.now(), stale=False, _processing=False)}) assert list(tmp_path.iterdir()) == [] @@ -751,14 +750,13 @@ def mock_func(): core.set_func(mock_func) - with patch("cachier.cores.pickle.tempfile.NamedTemporaryFile", side_effect=OSError("tempfile failed")), patch( - "cachier.cores.pickle.os.replace" - ) as mock_replace, patch("cachier.cores.pickle.os.remove") as mock_remove, pytest.raises( - OSError, match="tempfile failed" + with ( + patch("cachier.cores.pickle.tempfile.NamedTemporaryFile", side_effect=OSError("tempfile failed")), + patch("cachier.cores.pickle.os.replace") as mock_replace, + patch("cachier.cores.pickle.os.remove") as mock_remove, + pytest.raises(OSError, match="tempfile failed"), ): - core._save_cache( - {"key": CacheEntry(value="value", time=datetime.now(), stale=False, _processing=False)} - ) + core._save_cache({"key": CacheEntry(value="value", time=datetime.now(), stale=False, _processing=False)}) mock_replace.assert_not_called() mock_remove.assert_not_called() @@ -791,8 +789,9 @@ def mock_makedirs(path, exist_ok=False): raise PermissionError("temp dir not writable") raise AssertionError(f"Unexpected os.makedirs path: {path}") - with patch("cachier.cores.pickle.tempfile.gettempdir", return_value="/non-writable-temp"), patch( - "cachier.cores.pickle.os.makedirs", side_effect=mock_makedirs + with ( + patch("cachier.cores.pickle.tempfile.gettempdir", return_value="/non-writable-temp"), + patch("cachier.cores.pickle.os.makedirs", side_effect=mock_makedirs), ): assert core._shared_lock_fpath == os.path.join( fallback_lock_dir, @@ -827,8 +826,9 @@ def mock_makedirs(path, exist_ok=False): raise PermissionError("lock dir not writable") raise AssertionError(f"Unexpected os.makedirs path: {path}") - with patch("cachier.cores.pickle.tempfile.gettempdir", return_value="/non-writable-temp"), patch( - "cachier.cores.pickle.os.makedirs", side_effect=mock_makedirs + with ( + patch("cachier.cores.pickle.tempfile.gettempdir", return_value="/non-writable-temp"), + patch("cachier.cores.pickle.os.makedirs", side_effect=mock_makedirs), ): assert core._shared_lock_fpath == os.path.join(core.cache_dir, f".{cache_hash}{core._SHARED_LOCK_SUFFIX}")