From f2dfbb44a94c411cc653342a647eb9e811278919 Mon Sep 17 00:00:00 2001
From: Tamir Duberstein <tamird@gmail.com>
Date: Fri, 29 Mar 2024 15:51:51 +0000
Subject: [PATCH] Ensure proper cache initialization before writing

Writing cache data is interruptible; this prevents a pathological case
where interrupting a cache write can cause the cache directory to never
be properly initialized with its supporting files.

Unify `Cache.mkdir` with `Cache.set` while I'm here so the former also
properly initializes the cache directory.

Use json.dump instead of json.dumps + write for a tiny perf win.

Fixes #12167.
---
 src/_pytest/cacheprovider.py  | 46 ++++++++++++++++++++---------------
 testing/test_cacheprovider.py | 14 ++++++++---
 2 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/src/_pytest/cacheprovider.py b/src/_pytest/cacheprovider.py
index 81703ddac44..fa97373c09c 100755
--- a/src/_pytest/cacheprovider.py
+++ b/src/_pytest/cacheprovider.py
@@ -21,6 +21,7 @@
 from .reports import CollectReport
 from _pytest import nodes
 from _pytest._io import TerminalWriter
+from _pytest.compat import assert_never
 from _pytest.config import Config
 from _pytest.config import ExitCode
 from _pytest.config import hookimpl
@@ -123,6 +124,10 @@ def warn(self, fmt: str, *, _ispytest: bool = False, **args: object) -> None:
             stacklevel=3,
         )
 
+    def _mkdir(self, path: Path) -> None:
+        self._ensure_cache_dir_and_supporting_files()
+        path.mkdir(exist_ok=True, parents=True)
+
     def mkdir(self, name: str) -> Path:
         """Return a directory path object with the given name.
 
@@ -141,7 +146,7 @@ def mkdir(self, name: str) -> Path:
         if len(path.parts) > 1:
             raise ValueError("name is not allowed to contain path separators")
         res = self._cachedir.joinpath(self._CACHE_PREFIX_DIRS, path)
-        res.mkdir(exist_ok=True, parents=True)
+        self._mkdir(res)
         return res
 
     def _getvaluepath(self, key: str) -> Path:
@@ -178,20 +183,13 @@ def set(self, key: str, value: object) -> None:
         """
         path = self._getvaluepath(key)
         try:
-            if path.parent.is_dir():
-                cache_dir_exists_already = True
-            else:
-                cache_dir_exists_already = self._cachedir.exists()
-                path.parent.mkdir(exist_ok=True, parents=True)
+            self._mkdir(path.parent)
         except OSError as exc:
             self.warn(
                 f"could not create cache path {path}: {exc}",
                 _ispytest=True,
             )
             return
-        if not cache_dir_exists_already:
-            self._ensure_supporting_files()
-        data = json.dumps(value, ensure_ascii=False, indent=2)
         try:
             f = path.open("w", encoding="UTF-8")
         except OSError as exc:
@@ -201,19 +199,27 @@ def set(self, key: str, value: object) -> None:
             )
         else:
             with f:
-                f.write(data)
-
-    def _ensure_supporting_files(self) -> None:
-        """Create supporting files in the cache dir that are not really part of the cache."""
-        readme_path = self._cachedir / "README.md"
-        readme_path.write_text(README_CONTENT, encoding="UTF-8")
+                json.dump(value, f, ensure_ascii=False, indent=2)
 
-        gitignore_path = self._cachedir.joinpath(".gitignore")
-        msg = "# Created by pytest automatically.\n*\n"
-        gitignore_path.write_text(msg, encoding="UTF-8")
+    def _ensure_cache_dir_and_supporting_files(self) -> None:
+        """Create the cache dir and its supporting files."""
+        self._cachedir.mkdir(exist_ok=True, parents=True)
 
-        cachedir_tag_path = self._cachedir.joinpath("CACHEDIR.TAG")
-        cachedir_tag_path.write_bytes(CACHEDIR_TAG_CONTENT)
+        files: Iterable[tuple[str, str | bytes]] = (
+            ("README.md", README_CONTENT),
+            (".gitignore", "# Created by pytest automatically.\n*\n"),
+            ("CACHEDIR.TAG", CACHEDIR_TAG_CONTENT),
+        )
+        for file, content in files:
+            path = self._cachedir.joinpath(file)
+            if path.exists():
+                continue
+            if isinstance(content, str):
+                path.write_text(content)
+            elif isinstance(content, bytes):
+                path.write_bytes(content)
+            else:
+                assert_never(content)
 
 
 class LFPluginCollWrapper:
diff --git a/testing/test_cacheprovider.py b/testing/test_cacheprovider.py
index c020b77f978..3de95e7850a 100644
--- a/testing/test_cacheprovider.py
+++ b/testing/test_cacheprovider.py
@@ -1282,9 +1282,13 @@ def test_preserve_keys_order(pytester: Pytester) -> None:
     assert list(read_back.items()) == [("z", 1), ("b", 2), ("a", 3), ("d", 10)]
 
 
-def test_does_not_create_boilerplate_in_existing_dirs(pytester: Pytester) -> None:
+def test_does_not_create_overwrite_boilerplate(pytester: Pytester) -> None:
     from _pytest.cacheprovider import Cache
 
+    for filename in ("README.md", ".gitignore"):
+        with open(filename, "w") as f:
+            f.write(filename)
+
     pytester.makeini(
         """
         [pytest]
@@ -1296,8 +1300,12 @@ def test_does_not_create_boilerplate_in_existing_dirs(pytester: Pytester) -> Non
     cache.set("foo", "bar")
 
     assert os.path.isdir("v")  # cache contents
-    assert not os.path.exists(".gitignore")
-    assert not os.path.exists("README.md")
+
+    # unchanged
+    for filename in ("README.md", ".gitignore"):
+        with open(filename, "w") as f:
+            assert f.read() == filename
+            f.write("filename")
 
 
 def test_cachedir_tag(pytester: Pytester) -> None: