Concurrent execution of pip in multiple virtual environments fail (due to caching of packages)

[freand76]

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

• I agree to follow the PSF Code of Conduct.

[sbidoul]

Could this be a side effect of #11143 ?

cc/ @itamarst

[freand76]

Hi, I believe so.

[pfmoore]

It's not clear to me if CacheControl was ever guaranteed to be safe for concurrent use. Maybe this is something where the new implementation is simply more likely to trigger race conditions?

I'd be happy if this were fixed at the CacheControl level, but I'm not sure if it's something we should try to fix in pip. There's probably a lot of places where running multiple copies of pip in parallel is risky (for example doing multiple parallel installs to the same environment).

[itamarst]

There was some attempt to allow atomic updates of cache files, at least, so this may be a regression. I will take a look.

[itamarst]

I think I see an issue in CacheControl, at least. Previous cache format locking was tied to a file, and that was fine because both metadata and body were in the same file. Now they are two separate files so each gets locked separately and that's an opportunity for race conditions.

In Pip the locking logic is bypassed to use atomic replace via os.replace in a loop... but that leads to the same race condition due to using two separate files.

[itamarst]

There's a similar race condition when reading.

[itamarst]

Options:

1. Revert to version in 23.2.
2. On the CacheControl side, fix by adjusting the API and doing locking at a higher level... but pip disables the locking because it didn't want to depend on the deprecated lockfile project (Remove Lockfile #7023). CacheControl does support the filelock package, though, which is maintained, but that's another dependency to vendor.
3. In CacheControl, tweak the API and switch to a file format that is a single file, but in a better designed way so it doesn't suffer from the memory usage issues the original file format had.
4. Add locking to pip. I think this won't work on a per-download level since the relevant abstraction is in CacheControl, so it'd have to be on the cache level, which seems not ideal.
5. Decide parallel pip is not supported. Seems problematic to me. But maybe it was never supported?
6. ... something else? ...

[itamarst]

So... how does pip feel about vendoring https://pypi.org/project/filelock/? Beyond the vendoring issue, it feels like it might add some cross-platform brittleness.

[itamarst]

Based on the above I think I am leaning towards option 3, but I might be missing a more obvious solution, and this is partially about policy and architecture, so will await further feedback and keep thinking. I will implement whatever fix is chosen.

[pfmoore]

I don't have a major problem with vendoring lockfile, but I'm concerned that we might be re-introducing the issues mentioned in #6954 - I have no idea whether they were specific to lockfile or whether they are related to file locking in general.

I'd prefer option 3, simply because it's no work for pip 🙂 But I'm not against option 2, subject to the above.

I'm -1 on reverting. This feels like a relatively rare issue, and the benefits of the new cache are non-trivial.

I'm against option 4, because as you say, locking at pip's level would be messy and too broad. Also, we'd have to maintain the code and it'll be tricky to get right. That's precisely the sort of reason we defer stuff like this to vendored packages.

As far as option 5 is concerned, I think running pip in parallel has always been something of a "use at your own risk" exercise. I imagine that running pip install foo==1.0 and pip install foo==2.0 in parallel against the same environment would probably be subject to all sorts of race conditions, for example. While this issue is not as obvious a case of "just don't do that", it's still similar. So while I'm in favour of doing what we can to mitigate the problem, I don't want to set too strong a precedent that running pip in parallel is "supported", as such.

[itamarst]

Taking a step back, while there is a race condition... shouldn't the download key be unique per specific download? If so, the window for the race condition is actually very very short, the time between writing metadata and writing the body. And that might fixable just by swapping the order body and metadata are written.

Note I've gotten a corrupted hash once before, in older pip, and just assumed it was a memory bit flip somewhere in the CDN...

So maybe actual next step is for me to try to reproduce this.

[sbidoul]

Thanks for the rapid reaction and analysis, @itamarst !

I think running pip in parallel has always been something of a "use at your own risk" exercise.

I have personally always assumed the pip cache was multiprocess safe. FWIW, having several pip instances installing in different environments in parallel and sharing a common cache has always worked fine for my group at work.

From #4766 I gather we removed lockfile because it was unmaintained. In #7023 it is mentioned that it was replaced by a write-then-move approach. So my current understanding is that there was least intention to support this use case.

Solution 3 would be interesting, but it would change the cache format, right? Not necessarily a blocking point.

The filelock wheel is 11KB and is well maintained so I suppose it is fine to vendor it, if it makes the solution simpler and more robust.

One little problem with filelock is that the last version does not support python 3.7 anymore and pip still does. So if we vendor it in pip 23.3.x we'd need to vendor an older version (filelock 3.11 from April 2023).

[sbidoul]

Noting that protecting the cache from failed partial downloads is also important: #3792 (not sure if this still an issue).

[johannesacco]

Another idea might be to download to a temporary folder and then try to rename (with os.replace()) the folder once both files have been created. If the rename does not work you can assume that another process just downloaded it (and you can delete the temporary folder). The penalty here is that a file could be downloaded twice.

E.g.
.cache/pip/http-v2/f/c/f/6/e_tmp_g27322g/fcf6edeb293134ea65eddc3f995fc82af16b43e99cff2eeb972cdeb9
.cache/pip/http-v2/f/c/f/6/e_tmp_g27322g/fcf6edeb293134ea65eddc3f995fc82af16b43e99cff2eeb972cdeb9.body

os.replace(".cache/pip/http-v2/f/c/f/6/e_tmp_g27322g", ".cache/pip/http-v2/f/c/f/6/e")

[itamarst]

Failed partial downloads should be orthogonal to storage mechanism.

[itamarst]

OK, I couldn't reproduce in initial tests, but!

>>> hashlib.sha256(b"").hexdigest()
'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'

Notice that's the hash that was reported in original bug report. So this does seem like race condition: pip A sets metadata and then starts setting body, meanwhile pip B gets metadata and body, the body comes back as None, CacheControl says "OK it's empty body", and now it's broken.

I came up with a minimal, pip-only fix: when reading, if there is no body stored, don't return the metadata. Given os.replace() ensures atomicity, that should hopefully do the trick. That does leave the issue of the potential for inconsistent data in metadata and body since writes will still have a race condition, but that presumes the PyPI CDN is returning inconsistent data for different requests, which seems... bad? And already broken.

There should still be fixes in CacheControl, for other potential users, but that seems less urgent.

[notatallshaw]

```python
>>> hashlib.sha256(b"").hexdigest()
'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'

Notice that's the hash that was reported in original bug rep

Oh! I also had this error and this hash yesterday.

After investigating I realized I was getting the same error going to the previous version of Pip and therefore wasn't related to the Pip upgrade.

I found that our corporate firewall was blocking a wheel (specifically Flower 1.2.0), and rather than throwing an exception that the file failed to download I got this hash error.

Could this be the issue here? Can OP downgrade Pip, clear their cache, and rerun their tests.

[itamarst]

The downside of this fix is that the cache can no longer be used for non-downloads, e.g. redirects; is that a use case at all? It doesn't seem like it.

[freand76]

The merged solution works fine in our environment

[sbidoul]

pip 23.3.1 has been released with the fix for this.