diff --git a/changelog/12410.bugfix.rst b/changelog/12410.bugfix.rst new file mode 100644 index 000000000..a724d4011 --- /dev/null +++ b/changelog/12410.bugfix.rst @@ -0,0 +1,3 @@ +Fixed file write in `Cache.set()` to be atomic. + +Non-atomic write could cause issues when `Cache.set()` called from different pytest-xdist workers for the same key. Now it should be safe to use in combination with pytest-xdist. diff --git a/src/_pytest/cacheprovider.py b/src/_pytest/cacheprovider.py index 5aa8f4835..1ee0dc942 100755 --- a/src/_pytest/cacheprovider.py +++ b/src/_pytest/cacheprovider.py @@ -8,6 +8,7 @@ import json import os from pathlib import Path import tempfile +import time from typing import Dict from typing import final from typing import Generator @@ -75,6 +76,10 @@ class Cache: self._cachedir = cachedir self._config = config + # Note: there's no way to get the current umask atomically, eek. + self._umask = os.umask(0o022) + os.umask(self._umask) + @classmethod def for_config(cls, config: Config, *, _ispytest: bool = False) -> "Cache": """Create the Cache instance for a Config. @@ -124,6 +129,44 @@ class Cache: stacklevel=3, ) + def _write_atomic(self, path: Path, content: str) -> None: + tmpfile = tempfile.NamedTemporaryFile( + delete=False, dir=self._cachedir, mode="w", encoding="UTF-8" + ) + + with tmpfile: + tmpfile.write(content) + + # Reset permissions to the default, see #12308. + os.chmod(tmpfile.name, 0o666 - self._umask) + + # On Windows, replace() might fail with ERROR_ACCESS_DENIED (5) if + # the target file is open by another process. + # Retry with exponential backoff in this case. + retry_delay = 1 / (2**10) + deadline = time.perf_counter() + 5 + + while True: + try: + os.replace(tmpfile.name, path) + # Note: trying to remove tmpfile.name after successful replace() + # can cause a race condition, so it can't be done in 'finally:' + return + + except OSError as ex: + if getattr(ex, "winerror", None) != 5 or time.perf_counter() > deadline: + os.remove(tmpfile.name) + raise + + except BaseException: + os.remove(tmpfile.name) + raise + + time.sleep(max(0, min(retry_delay, deadline - time.perf_counter()))) + + if retry_delay < 0.25: + retry_delay *= 2 + def _mkdir(self, path: Path) -> None: self._ensure_cache_dir_and_supporting_files() path.mkdir(exist_ok=True, parents=True) @@ -192,15 +235,12 @@ class Cache: return data = json.dumps(value, ensure_ascii=False, indent=2) try: - f = path.open("w", encoding="UTF-8") + self._write_atomic(path, data) except OSError as exc: self.warn( f"cache could not write path {path}: {exc}", _ispytest=True, ) - else: - with f: - f.write(data) def _ensure_cache_dir_and_supporting_files(self) -> None: """Create the cache dir and its supporting files.""" @@ -215,10 +255,7 @@ class Cache: path = Path(newpath) # Reset permissions to the default, see #12308. - # Note: there's no way to get the current umask atomically, eek. - umask = os.umask(0o022) - os.umask(umask) - path.chmod(0o777 - umask) + path.chmod(0o777 - self._umask) with open(path.joinpath("README.md"), "xt", encoding="UTF-8") as f: f.write(README_CONTENT) diff --git a/testing/test_cacheprovider.py b/testing/test_cacheprovider.py index 08158f619..975a41052 100644 --- a/testing/test_cacheprovider.py +++ b/testing/test_cacheprovider.py @@ -3,11 +3,14 @@ from enum import Enum import os from pathlib import Path import shutil +import subprocess +import sys from typing import Any from typing import Generator from typing import List from typing import Sequence from typing import Tuple +from typing import Union from _pytest.compat import assert_never from _pytest.config import ExitCode @@ -1360,3 +1363,103 @@ def test_cachedir_tag(pytester: Pytester) -> None: def test_clioption_with_cacheshow_and_help(pytester: Pytester) -> None: result = pytester.runpytest("--cache-show", "--help") assert result.ret == 0 + + +def test_key_permissions(pytester: Pytester) -> None: + ini_path = pytester.makeini("[pytest]") + config = pytester.parseconfigure() + assert config.cache is not None + + config.cache.set("test/key", "value") + key_path = config.cache._getvaluepath("test/key") + + assert key_path.stat().st_mode == ini_path.stat().st_mode + + +def test_write_fail_permissions(pytester: Pytester) -> None: + pytester.makeini("[pytest]") + config = pytester.parseconfigure() + assert config.cache is not None + + config.cache.set("test/key", "initial") + key_path = config.cache._getvaluepath("test/key") + + old_key_file_mode = key_path.stat().st_mode + old_key_dir_mode = key_path.parent.stat().st_mode + + try: + key_path.chmod(0) + key_path.parent.chmod(0) + + with pytest.warns(pytest.PytestCacheWarning) as warns: + config.cache.set("test/key", "value") + + assert len(warns) == 1 + assert str(warns[0].message).startswith("cache could not write path") + + finally: + key_path.parent.chmod(old_key_dir_mode) + key_path.chmod(old_key_file_mode) + + +def test_concurrent_write(pytester: Pytester, monkeypatch: MonkeyPatch) -> None: + pytester.makeini("[pytest]") + config = pytester.parseconfigure() + assert config.cache is not None + + config.cache.set("test/key", "initial") + key_path = config.cache._getvaluepath("test/key") + + code_file = pytester.makepyfile( + f""" + import sys + + f = open({str(key_path)!r}, mode="rb") + print("opened", flush=True) + + line = sys.stdin.readline() + if line != "close\\n": + raise ValueError(f"Expected 'close', got {{line!r}}") + + f.close() + print("done", flush=True) + """ + ) + + old_replace = os.replace + replace_attempts = 0 + + def mock_replace( + src: "Union[str, os.PathLike[str]]", + dst: "Union[str, os.PathLike[str]]", + ) -> None: + try: + old_replace(src, dst) + finally: + if dst == key_path: + nonlocal replace_attempts + replace_attempts += 1 + if replace_attempts == 1: + print("close", file=child.stdin, flush=True) + + child = pytester.popen( + [sys.executable, code_file], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=sys.stderr, + encoding="utf-8", + ) + + with child: + assert child.stdout.readline() == "opened\n" + monkeypatch.setattr("os.replace", mock_replace) + config.cache.set("test/key", "value") + assert child.stdout.read() == "done\n" + + assert child.returncode == 0 + assert config.cache.get("test/key", None) == "value" + + if sys.platform.startswith("win"): + assert replace_attempts > 1 + else: + assert replace_attempts == 1