Skip to content

Commit 9eb0755

Browse files
committed
Make clear_caches safe under concurrent pip-compile --rebuild
Two ``pip-compile --rebuild`` invocations sharing a ``cache_dir`` race inside ``PyPIRepository.clear_caches``: both call ``rmtree`` on the same download directory, and one will be midway through deleting it while the other tries to walk it for fresh downloads, leaving a partial tree the second resolver then trips over. ``ignore_errors=True`` hides the error but not the corruption. Atomically rename the download directory to a per-PID sibling before deleting it. The atomic rename is the only step a concurrent peer can observe, so the peer either still sees the original directory or sees nothing at all, never partial state. The renamed copy is then removed opportunistically; if a racing peer beat us to the rename, the ``OSError`` is swallowed because the cache wipe has effectively already happened.
1 parent ef924e9 commit 9eb0755

4 files changed

Lines changed: 65 additions & 1 deletion

File tree

changelog.d/2393.bugfix.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
:meth:`piptools.repositories.PyPIRepository.clear_caches` is now safe to invoke
2+
concurrently against the same ``cache_dir``: the download directory is renamed
3+
atomically before deletion, so a parallel :command:`pip-compile --rebuild`
4+
lookup either still sees the old directory or no directory at all and never
5+
trips over a half-deleted tree (:issue:`2393`) -- by :user:`gaborbernat`.

piptools/repositories/pypi.py

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,21 @@ def __init__(self, pip_args: list[str], cache_dir: str):
104104
)
105105

106106
def clear_caches(self) -> None:
107-
rmtree(self._download_dir, ignore_errors=True)
107+
# Two ``pip-compile --rebuild`` runs against the same ``cache_dir``
108+
# would otherwise have one ``rmtree`` wipe the other's in-flight
109+
# downloads, leaving the second resolver staring at a half-deleted
110+
# tree. Atomically rename out of the way first so a concurrent
111+
# lookup either still sees the old directory or sees nothing at
112+
# all, never partial state. The renamed copy is then deleted
113+
# opportunistically.
114+
if not os.path.exists(self._download_dir):
115+
return
116+
stale = f"{self._download_dir}.stale-{os.getpid()}"
117+
try:
118+
os.replace(self._download_dir, stale)
119+
except OSError:
120+
return
121+
rmtree(stale, ignore_errors=True)
108122

109123
@property
110124
def options(self) -> optparse.Values:

pyproject.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ changelog = "https://github.com/jazzband/pip-tools/releases"
5858
[project.optional-dependencies]
5959
testing = [
6060
"pytest >= 7.2.0",
61+
"pytest-mock >= 3.15.1",
6162
"pytest-rerunfailures",
6263
"pytest-xdist",
6364
"tomli-w",

tests/test_repository_pypi.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
from __future__ import annotations
22

33
import os
4+
from pathlib import Path
45
from unittest import mock
56

67
import pytest
78
from pip._internal.models.candidate import InstallationCandidate
89
from pip._internal.models.link import Link
910
from pip._internal.utils.urls import path_to_url
1011
from pip._vendor.requests import HTTPError, Session
12+
from pytest_mock import MockerFixture
1113

1214
from piptools.repositories import PyPIRepository
1315
from piptools.repositories.pypi import open_local_or_remote_file
@@ -169,6 +171,48 @@ def test_pip_cache_dir_is_empty(from_line, tmpdir):
169171
assert not pypi_repository.options.cache_dir
170172

171173

174+
@pytest.fixture
175+
def repo_with_cache_dir(tmp_path: Path) -> PyPIRepository:
176+
return PyPIRepository([], cache_dir=str(tmp_path / "cache"))
177+
178+
179+
def test_clear_caches_removes_existing_download_dir(
180+
repo_with_cache_dir: PyPIRepository,
181+
) -> None:
182+
download_dir = Path(repo_with_cache_dir._download_dir)
183+
download_dir.mkdir(parents=True, exist_ok=True)
184+
(download_dir / "marker").write_text("x")
185+
186+
repo_with_cache_dir.clear_caches()
187+
188+
assert not download_dir.exists()
189+
190+
191+
def test_clear_caches_when_download_dir_missing_is_a_no_op(
192+
repo_with_cache_dir: PyPIRepository,
193+
) -> None:
194+
assert not Path(repo_with_cache_dir._download_dir).exists()
195+
196+
repo_with_cache_dir.clear_caches()
197+
198+
assert not Path(repo_with_cache_dir._download_dir).exists()
199+
200+
201+
def test_clear_caches_swallows_replace_failure(
202+
repo_with_cache_dir: PyPIRepository, mocker: MockerFixture
203+
) -> None:
204+
Path(repo_with_cache_dir._download_dir).mkdir(parents=True, exist_ok=True)
205+
mocker.patch(
206+
"piptools.repositories.pypi.os.replace",
207+
side_effect=OSError("racing peer renamed first"),
208+
)
209+
rmtree = mocker.patch("piptools.repositories.pypi.rmtree")
210+
211+
repo_with_cache_dir.clear_caches()
212+
213+
rmtree.assert_not_called()
214+
215+
172216
@pytest.mark.parametrize(
173217
("project_data", "expected_hashes"),
174218
(

0 commit comments

Comments
 (0)