Repository: pypa/pip

Issue:
Concurrent execution of pip in multiple virtual environments fail (due to caching of packages)
### Description

If multiple virtual environments are installing the same package at the same time one of them can/will fail with a cache error similar to the one below.

i.e. one virtual envioronment is downloading the file and caching it. Another virtual environment will try to access the cached file **before** the download has finished. This feature was working in 23.2.1.

**ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE. If you have updated the package versions, please update the hashes. Otherwise, examine the package contents carefully; someone may have tampered with them.
    numpy>=1.20 from https://files.pythonhosted.org/packages/98/5d/5738903efe0ecb73e51eb44feafba32bdba2081263d40c5043568ff60faf/numpy-1.24.4-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl:
        Expected sha256 901677b9c6e0973ed91ece5a79fad3c42dafd884e1d7299cf5c392a7e7c62398
             Got        e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855**


### Expected behavior

I expected the virtual environment either to download its own version of the package OR to wait for the cached version to be completed before accessing it.

### pip version

23.3

### Python version

3.8.10

### OS

linux/ubuntu 20.04

### How to Reproduce

Have multiple virtual environment running in parallel where at least some of the packages are shared between them.

### Output

**ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE. If you have updated the package versions, please update the hashes. Otherwise, examine the package contents carefully; someone may have tampered with them.
    numpy>=1.20 from https://files.pythonhosted.org/packages/98/5d/5738903efe0ecb73e51eb44feafba32bdba2081263d40c5043568ff60faf/numpy-1.24.4-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl:
        Expected sha256 901677b9c6e0973ed91ece5a79fad3c42dafd884e1d7299cf5c392a7e7c62398
             Got        e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855**


### Code of Conduct

- [X] I agree to follow the [PSF Code of Conduct](https://www.python.org/psf/conduct/).

Discussion:
pfmoore: This seems like a reasonable approach. I can't confirm that the assumption you state is valid, but it seems correct to me. I think I'd be happy to say that we don't need to support URLs that serve different content at different times (although I don't have experience with some of the big development infrastructures that people work with, so it's possible I'm wrong).
itamarst: I am going to add references to the upstream issue, and document the invariant at least.
itamarst: Even if the writes assumption is wrong in some edge cases, this doesn't make things worse compared to 23.3 in those edge cases, and it makes things better everywhere else.
itamarst: This PR also fixes the case where pip crashes half-way through writing the cache entry, i.e. after writing metadata but before writing the body.

Assuming you're happy with the code, it seems like an immediate improvement, and a long-term fix can be done once CacheControl has fixes on its side for the two issues I filed.

Patch:
diff --git a/news/12361.bugfix.rst b/news/12361.bugfix.rst
new file mode 100644
index 000000000..59575e80b
--- /dev/null
+++ b/news/12361.bugfix.rst
@@ -0,0 +1,1 @@
+Fix bug where installing the same package at the same time with multiple pip processes could fail.
diff --git a/src/pip/_internal/network/cache.py b/src/pip/_internal/network/cache.py
index a4d136205..4d0fb545d 100644
--- a/src/pip/_internal/network/cache.py
+++ b/src/pip/_internal/network/cache.py
@@ -33,6 +33,18 @@ class SafeFileCache(SeparateBodyBaseCache):
     """
     A file based cache which is safe to use even when the target directory may
     not be accessible or writable.
+
+    There is a race condition when two processes try to write and/or read the
+    same entry at the same time, since each entry consists of two separate
+    files (https://github.com/psf/cachecontrol/issues/324).  We therefore have
+    additional logic that makes sure that both files to be present before
+    returning an entry; this fixes the read side of the race condition.
+
+    For the write side, we assume that the server will only ever return the
+    same data for the same URL, which ought to be the case for files pip is
+    downloading.  PyPI does not have a mechanism to swap out a wheel for
+    another wheel, for example.  If this assumption is not true, the
+    CacheControl issue will need to be fixed.
     """
 
     def __init__(self, directory: str) -> None:
@@ -49,9 +61,13 @@ class SafeFileCache(SeparateBodyBaseCache):
         return os.path.join(self.directory, *parts)
 
     def get(self, key: str) -> Optional[bytes]:
-        path = self._get_cache_path(key)
+        # The cache entry is only valid if both metadata and body exist.
+        metadata_path = self._get_cache_path(key)
+        body_path = metadata_path + ".body"
+        if not (os.path.exists(metadata_path) and os.path.exists(body_path)):
+            return None
         with suppressed_cache_errors():
-            with open(path, "rb") as f:
+            with open(metadata_path, "rb") as f:
                 return f.read()
 
     def _write(self, path: str, data: bytes) -> None:
@@ -77,9 +93,13 @@ class SafeFileCache(SeparateBodyBaseCache):
             os.remove(path + ".body")
 
     def get_body(self, key: str) -> Optional[BinaryIO]:
-        path = self._get_cache_path(key) + ".body"
+        # The cache entry is only valid if both metadata and body exist.
+        metadata_path = self._get_cache_path(key)
+        body_path = metadata_path + ".body"
+        if not (os.path.exists(metadata_path) and os.path.exists(body_path)):
+            return None
         with suppressed_cache_errors():
-            return open(path, "rb")
+            return open(body_path, "rb")
 
     def set_body(self, key: str, body: bytes) -> None:
         path = self._get_cache_path(key) + ".body"


Test patch:
diff --git a/tests/unit/test_network_cache.py b/tests/unit/test_network_cache.py
index aa849f3b0..6a816b300 100644
--- a/tests/unit/test_network_cache.py
+++ b/tests/unit/test_network_cache.py
@@ -27,6 +27,11 @@ class TestSafeFileCache:
         cache = SafeFileCache(os.fspath(cache_tmpdir))
         assert cache.get("test key") is None
         cache.set("test key", b"a test string")
+        # Body hasn't been stored yet, so the entry isn't valid yet
+        assert cache.get("test key") is None
+
+        # With a body, the cache entry is valid:
+        cache.set_body("test key", b"body")
         assert cache.get("test key") == b"a test string"
         cache.delete("test key")
         assert cache.get("test key") is None
@@ -35,6 +40,12 @@ class TestSafeFileCache:
         cache = SafeFileCache(os.fspath(cache_tmpdir))
         assert cache.get_body("test key") is None
         cache.set_body("test key", b"a test string")
+        # Metadata isn't available, so the entry isn't valid yet (this
+        # shouldn't happen, but just in case)
+        assert cache.get_body("test key") is None
+
+        # With metadata, the cache entry is valid:
+        cache.set("test key", b"metadata")
         body = cache.get_body("test key")
         assert body is not None
         with body: