Skip to content

Commit 5820e76

Browse files
committed
Avoid half-deleted cache trees on concurrent pip-compile --rebuild
Two ``pip-compile --rebuild`` invocations sharing a ``cache_dir`` race inside ``PyPIRepository.clear_caches``. Both call ``shutil.rmtree(self._download_dir, ignore_errors=True)`` on the same path, and the second resolver walks a tree the first started removing. ``ignore_errors=True`` masks the error but not the corruption. The patch swaps the in-place delete for an atomic ``os.replace`` to a per-PID sibling directory followed by ``rmtree`` of the renamed copy. ``os.replace`` is atomic on POSIX and NTFS, so a concurrent peer sees either the old directory or no directory, never a half-deleted tree. Scope is intentionally narrow: this fixes one observable symptom of #2393, not the wider race where a process is already using the cache and a peer ``--rebuild`` later renames the directory out from under it. Full concurrent-rebuild safety needs locking around the resolve and is left for future work.
1 parent 59c1f32 commit 5820e76

4 files changed

Lines changed: 65 additions & 1 deletion

File tree

changelog.d/2393.bugfix.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
Atomically rename the download directory before deleting it in
2+
{meth}`piptools.repositories.PyPIRepository.clear_caches` so a parallel
3+
{command}`pip-compile --rebuild` no longer trips over a half-deleted
4+
tree. This narrows one symptom of #2393 and does not fully serialise
5+
concurrent ``--rebuild`` runs; broader cache locking remains future
6+
work -- {user}`gaborbernat`.

piptools/repositories/pypi.py

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,20 @@ 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+
# Atomic rename + opportunistic rmtree narrows one specific race:
108+
# two ``pip-compile --rebuild`` runs on the same ``cache_dir`` no
109+
# longer leave the second resolver staring at a tree the first
110+
# started removing. It does not serialise concurrent ``--rebuild``
111+
# runs as a whole; full concurrent-rebuild safety needs locking
112+
# around the resolve, which is out of scope here.
113+
if not os.path.exists(self._download_dir):
114+
return
115+
stale = f"{self._download_dir}.stale-{os.getpid()}"
116+
try:
117+
os.replace(self._download_dir, stale)
118+
except OSError:
119+
return
120+
rmtree(stale, ignore_errors=True)
108121

109122
@property
110123
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)