cacheprovider: write files atomically

Make `Cache.set()` safe to use together with xdist.

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.

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.
+            path.chmod(0o777 - self._umask)
-            umask = os.umask(0o022)
-            os.umask(umask)
-            path.chmod(0o777 - umask)
 
             with open(path.joinpath("README.md"), "xt", encoding="UTF-8") as f:
                 f.write(README_CONTENT)

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