-
-
Notifications
You must be signed in to change notification settings - Fork 664
Avoid half-deleted cache trees on concurrent pip-compile --rebuild #2384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| 2393.bugfix.md |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| {meth}`piptools.repositories.PyPIRepository.clear_caches` now atomically | ||
| renames the download directory before deleting it, so a parallel | ||
| {command}`pip-compile --rebuild` no longer trips over a half-deleted | ||
| tree. The fix narrows one observable symptom; it does not fully | ||
| serialise concurrent {command}`pip-compile --rebuild` runs, and broader | ||
| cache locking remains future work -- by {user}`gaborbernat`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,7 +104,25 @@ def __init__(self, pip_args: list[str], cache_dir: str): | |
| ) | ||
|
|
||
| def clear_caches(self) -> None: | ||
| rmtree(self._download_dir, ignore_errors=True) | ||
| # Atomic rename + opportunistic rmtree narrows one specific race: | ||
| # two ``pip-compile --rebuild`` runs on the same ``cache_dir`` no | ||
| # longer leave the second resolver staring at a tree the first | ||
| # started removing. It does not serialise concurrent ``--rebuild`` | ||
| # runs as a whole; full concurrent-rebuild safety needs locking | ||
| # around the resolve, which is out of scope here. | ||
| if not os.path.exists(self._download_dir): | ||
| return | ||
| old_downloads_dir = f"{self._download_dir}.stale-{os.getpid()}" | ||
| try: | ||
| os.replace(self._download_dir, old_downloads_dir) | ||
| except OSError as os_err: | ||
| # A racing peer renamed the tree first, or the OS refused the | ||
| # move (cross-device, permissions). The directory is no longer | ||
| # ours to clear; log at -v since the suppressed OSError is | ||
| # otherwise invisible and its error codes shift across platforms. | ||
| log.debug(f"clear_caches skipped {self._download_dir!s}: {os_err}") | ||
| return | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably needs to be logged in the very verbose mode. Especially since any arbitrary cc @sirosen WDYT? |
||
| rmtree(old_downloads_dir, ignore_errors=True) | ||
|
|
||
| @property | ||
| def options(self) -> optparse.Values: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,15 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| from pathlib import Path | ||
| from unittest import mock | ||
|
|
||
| import pytest | ||
| from pip._internal.models.candidate import InstallationCandidate | ||
| from pip._internal.models.link import Link | ||
| from pip._internal.utils.urls import path_to_url | ||
| from pip._vendor.requests import HTTPError, Session | ||
| from pytest_mock import MockerFixture | ||
|
|
||
| from piptools.repositories import PyPIRepository | ||
| from piptools.repositories.pypi import open_local_or_remote_file | ||
|
|
@@ -169,6 +171,52 @@ def test_pip_cache_dir_is_empty(from_line, tmpdir): | |
| assert not pypi_repository.options.cache_dir | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def repo_with_cache_dir(tmp_path: Path) -> PyPIRepository: | ||
| return PyPIRepository([], cache_dir=str(tmp_path / "cache")) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Side note: |
||
|
|
||
|
|
||
| def test_clear_caches_removes_existing_download_dir( | ||
| repo_with_cache_dir: PyPIRepository, | ||
| ) -> None: | ||
| download_dir = Path(repo_with_cache_dir._download_dir) | ||
| download_dir.mkdir(parents=True, exist_ok=True) | ||
| (download_dir / "marker").write_text("x") | ||
|
|
||
| repo_with_cache_dir.clear_caches() | ||
|
|
||
| assert not download_dir.exists() | ||
|
|
||
|
|
||
| def test_clear_caches_when_download_dir_missing_is_a_no_op( | ||
| repo_with_cache_dir: PyPIRepository, | ||
| ) -> None: | ||
| assert not Path(repo_with_cache_dir._download_dir).exists() | ||
|
|
||
| repo_with_cache_dir.clear_caches() | ||
|
|
||
| assert not Path(repo_with_cache_dir._download_dir).exists() | ||
|
|
||
|
|
||
| def test_clear_caches_swallows_replace_failure( | ||
| repo_with_cache_dir: PyPIRepository, mocker: MockerFixture | ||
| ) -> None: | ||
| download_dir = Path(repo_with_cache_dir._download_dir) | ||
| download_dir.mkdir(parents=True, exist_ok=True) | ||
| replace = mocker.patch( | ||
| "piptools.repositories.pypi.os.replace", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you're already integrating |
||
| side_effect=OSError("racing peer renamed first"), | ||
| ) | ||
| rmtree = mocker.patch("piptools.repositories.pypi.rmtree") | ||
|
|
||
| repo_with_cache_dir.clear_caches() | ||
|
|
||
| # The rename targets the live download dir; the OSError short-circuits | ||
| # before the rmtree so a racing peer keeps ownership of the tree. | ||
| assert replace.call_args.args[0] == str(download_dir) | ||
| rmtree.assert_not_called() | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("project_data", "expected_hashes"), | ||
| ( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.