Repository: pypa/pip
Pull request #11394 (2022-08-19 14:35:13)

Ignore errors in temporary directory cleanup
<!---
Thank you for your soon to be pull request. Before you submit this, please
double check to make sure that you've added a news file fragment. In pip we
generate our NEWS.rst from multiple news fragment files, and all pull requests
require either a news file fragment or a marker to indicate they don't require
one.

To read more about adding a news file fragment for your PR, please check out
our documentation at: https://pip.pypa.io/en/latest/development/contributing/#news-entries
-->

Pip frequently fails on windows with a PermissionError in temporary directory cleanup after it has already successfully installed upgraded/uninstalled a package. 

E.g if you have `import matplotlib` in one python session and run 
```
> pip install matplotlib==3.4.3
```
in another, you get
```
...
Installing collected packages: matplotlib
  Attempting uninstall: matplotlib
    Found existing installation: matplotlib 3.4.3
    Uninstalling matplotlib-3.4.3:
      Successfully uninstalled matplotlib-3.4.3
ERROR: Could not install packages due to an OSError: [WinError 5] Access is denied: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~4tplotlib\\ft2font.cp39-win_amd64.pyd'
Check the permissions.
```
(note the '\\\\**~4**tplotlib\\\\' name) indicating this is failure to remove an temp backup; a new package is already fully installed at this point)

With this applied
```
Installing collected packages: matplotlib
  Attempting uninstall: matplotlib
    Found existing installation: matplotlib 3.4.2
    Uninstalling matplotlib-3.4.2:
      Successfully uninstalled matplotlib-3.4.2
  WARNING: Failed to remove a temporary file 'C:\Users\ales\.envs\Lib\site-packages\~-tplotlib\ft2font.cp39-win_amd64.pyd' due to PermissionError: [WinError 5] Access is denied: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~-tplotlib\\ft2font.cp39-win_amd64.pyd'.
  You can safely remove it manually.
  WARNING: Failed to remove a temporary file 'C:\Users\ales\.envs\Lib\site-packages\~-tplotlib\_c_internal_utils.cp39-win_amd64.pyd' due to PermissionError: [WinError 5] Access is denied: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~-tplotlib\\_c_internal_utils.cp39-win_amd64.pyd'.
  You can safely remove it manually.
  WARNING: Failed to remove a temporary file 'C:\Users\ales\.envs\Lib\site-packages\~-tplotlib\_path.cp39-win_amd64.pyd' due to PermissionError: [WinError 5] Access is denied: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~-tplotlib\\_path.cp39-win_amd64.pyd'.
  You can safely remove it manually.
  WARNING: Failed to remove a temporary file 'C:\Users\ales\.envs\Lib\site-packages\~-tplotlib' due to OSError: [WinError 145] The directory is not empty: 'C:\\Users\\ales\\.envs\\Lib\\site-packages\\~-tplotlib'.
  You can safely remove it manually.
Successfully installed matplotlib-3.4.3
```

This also fixes https://github.com/pypa/pip/issues/6327 (similar problem on nfs)


uranusjr: I wonder how viable it’d be to aggregate these removal failures and show just one warning message before the process exits instead. This can be quite spammy if a package contains a lot of files cannot be removed.
just-a-mango: Is this even viable ? As @uranusjr said, it would be a mess when installing packages containing many files, and would spam the console. Perhaps leave this as an option ?
ales-erjavec: I have changed it so only a single warning is displayed for a temporary directory.
```
...
Installing collected packages: matplotlib
  Attempting uninstall: matplotlib
    Found existing installation: matplotlib 3.4.3
    Uninstalling matplotlib-3.4.3:
      Successfully uninstalled matplotlib-3.4.3
  WARNING: Failed to remove contents of a temporary directory 'C:\Users\ales\.envs\Lib\site-packages\~6tplotlib'.
  You can safely remove it manually.
Successfully installed matplotlib-3.4.2
```
This is still done when `TempDirectory.cleanup` is called. 

`TempDirectory` does have means of being `globally_managed` (to cleanup at exit?), but the 'uninstall backups' do not use this and I do not feel comfortable changing this.
tmacecode: Hi. Is there anything blocking this PR being merged? This will fix a problem we are hitting regularly. Many thanks

Patch:
diff --git a/news/11394.bugfix.rst b/news/11394.bugfix.rst
new file mode 100644
index 000000000..9f2501db4
--- /dev/null
+++ b/news/11394.bugfix.rst
@@ -0,0 +1,1 @@
+Ignore errors in temporary directory cleanup (show a warning instead).
diff --git a/src/pip/_internal/utils/misc.py b/src/pip/_internal/utils/misc.py
index bd191c4e1..b7b32f0f8 100644
--- a/src/pip/_internal/utils/misc.py
+++ b/src/pip/_internal/utils/misc.py
@@ -11,6 +11,7 @@ import stat
 import sys
 import sysconfig
 import urllib.parse
+from functools import partial
 from io import StringIO
 from itertools import filterfalse, tee, zip_longest
 from types import TracebackType
@@ -123,33 +124,66 @@ def get_prog() -> str:
 # Retry every half second for up to 3 seconds
 # Tenacity raises RetryError by default, explicitly raise the original exception
 @retry(reraise=True, stop=stop_after_delay(3), wait=wait_fixed(0.5))
-def rmtree(dir: str, ignore_errors: bool = False) -> None:
+def rmtree(
+    dir: str,
+    ignore_errors: bool = False,
+    onexc: Optional[Callable[[Any, Any, Any], Any]] = None,
+) -> None:
+    if ignore_errors:
+        onexc = _onerror_ignore
+    elif onexc is None:
+        onexc = _onerror_reraise
     if sys.version_info >= (3, 12):
-        shutil.rmtree(dir, ignore_errors=ignore_errors, onexc=rmtree_errorhandler)
+        shutil.rmtree(dir, onexc=partial(rmtree_errorhandler, onexc=onexc))
     else:
-        shutil.rmtree(dir, ignore_errors=ignore_errors, onerror=rmtree_errorhandler)
+        shutil.rmtree(dir, onerror=partial(rmtree_errorhandler, onexc=onexc))
+
+
+def _onerror_ignore(*_args: Any) -> None:
+    pass
+
+
+def _onerror_reraise(*_args: Any) -> None:
+    raise
 
 
 def rmtree_errorhandler(
-    func: Callable[..., Any], path: str, exc_info: Union[ExcInfo, BaseException]
+    func: Callable[..., Any],
+    path: str,
+    exc_info: Union[ExcInfo, BaseException],
+    *,
+    onexc: Callable[..., Any] = _onerror_reraise,
 ) -> None:
-    """On Windows, the files in .svn are read-only, so when rmtree() tries to
-    remove them, an exception is thrown.  We catch that here, remove the
-    read-only attribute, and hopefully continue without problems."""
+    """
+    `rmtree` error handler to 'force' a file remove (i.e. like `rm -f`).
+
+    * If a file is readonly then it's write flag is set and operation is
+      retried.
+
+    * `onerror` is the original callback from `rmtree(... onerror=onerror)`
+      that is chained at the end if the "rm -f" still fails.
+    """
     try:
-        has_attr_readonly = not (os.stat(path).st_mode & stat.S_IWRITE)
+        st_mode = os.stat(path).st_mode
     except OSError:
         # it's equivalent to os.path.exists
         return
 
-    if has_attr_readonly:
+    if not st_mode & stat.S_IWRITE:
         # convert to read/write
-        os.chmod(path, stat.S_IWRITE)
-        # use the original function to repeat the operation
-        func(path)
-        return
-    else:
-        raise
+        try:
+            os.chmod(path, st_mode | stat.S_IWRITE)
+        except OSError:
+            pass
+        else:
+            # use the original function to repeat the operation
+            try:
+                func(path)
+                return
+            except OSError:
+                pass
+
+    onexc(func, path, exc_info)
 
 
 def display_path(path: str) -> str:
diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py
index 8ee8a1cb1..99d1ba834 100644
--- a/src/pip/_internal/utils/temp_dir.py
+++ b/src/pip/_internal/utils/temp_dir.py
@@ -3,8 +3,20 @@ import itertools
 import logging
 import os.path
 import tempfile
+import traceback
 from contextlib import ExitStack, contextmanager
-from typing import Any, Dict, Generator, Optional, TypeVar, Union
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    Generator,
+    List,
+    Optional,
+    Tuple,
+    Type,
+    TypeVar,
+    Union,
+)
 
 from pip._internal.utils.misc import enum, rmtree
 
@@ -106,6 +118,7 @@ class TempDirectory:
         delete: Union[bool, None, _Default] = _default,
         kind: str = "temp",
         globally_managed: bool = False,
+        ignore_cleanup_errors: bool = True,
     ):
         super().__init__()
 
@@ -128,6 +141,7 @@ class TempDirectory:
         self._deleted = False
         self.delete = delete
         self.kind = kind
+        self.ignore_cleanup_errors = ignore_cleanup_errors
 
         if globally_managed:
             assert _tempdir_manager is not None
@@ -170,7 +184,42 @@ class TempDirectory:
         self._deleted = True
         if not os.path.exists(self._path):
             return
-        rmtree(self._path)
+
+        errors: List[BaseException] = []
+
+        def onerror(
+            func: Callable[[str], Any],
+            path: str,
+            exc_info: Tuple[Type[BaseException], BaseException, Any],
+        ) -> None:
+            """Log a warning for a `rmtree` error and continue"""
+            exc_val = "\n".join(traceback.format_exception_only(*exc_info[:2]))
+            exc_val = exc_val.rstrip()  # remove trailing new line
+            if func in (os.unlink, os.remove, os.rmdir):
+                logger.debug(
+                    "Failed to remove a temporary file '%s' due to %s.\n",
+                    path,
+                    exc_val,
+                )
+            else:
+                logger.debug("%s failed with %s.", func.__qualname__, exc_val)
+            errors.append(exc_info[1])
+
+        if self.ignore_cleanup_errors:
+            try:
+                # first try with tenacity; retrying to handle ephemeral errors
+                rmtree(self._path, ignore_errors=False)
+            except OSError:
+                # last pass ignore/log all errors
+                rmtree(self._path, onexc=onerror)
+            if errors:
+                logger.warning(
+                    "Failed to remove contents in a temporary directory '%s'.\n"
+                    "You can safely remove it manually.",
+                    self._path,
+                )
+        else:
+            rmtree(self._path)
 
 
 class AdjacentTempDirectory(TempDirectory):


Test patch:
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index 450081cfd..d3b0d32d1 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -257,9 +257,13 @@ def test_rmtree_errorhandler_reraises_error(tmpdir: Path) -> None:
     except RuntimeError:
         # Make sure the handler reraises an exception
         with pytest.raises(RuntimeError, match="test message"):
-            # Argument 3 to "rmtree_errorhandler" has incompatible type "None"; expected
-            # "Tuple[Type[BaseException], BaseException, TracebackType]"
-            rmtree_errorhandler(mock_func, path, None)  # type: ignore[arg-type]
+            # Argument 3 to "rmtree_errorhandler" has incompatible type
+            # "Union[Tuple[Type[BaseException], BaseException, TracebackType],
+            # Tuple[None, None, None]]"; expected "Tuple[Type[BaseException],
+            # BaseException, TracebackType]"
+            rmtree_errorhandler(
+                mock_func, path, sys.exc_info()  # type: ignore[arg-type]
+            )
 
     mock_func.assert_not_called()
 
diff --git a/tests/unit/test_utils_temp_dir.py b/tests/unit/test_utils_temp_dir.py
index 4a656d23a..a6cd0d0e5 100644
--- a/tests/unit/test_utils_temp_dir.py
+++ b/tests/unit/test_utils_temp_dir.py
@@ -4,6 +4,7 @@ import stat
 import tempfile
 from pathlib import Path
 from typing import Any, Iterator, Optional, Union
+from unittest import mock
 
 import pytest
 
@@ -274,3 +275,25 @@ def test_tempdir_registry_lazy(should_delete: bool) -> None:
             registry.set_delete("test-for-lazy", should_delete)
             assert os.path.exists(path)
         assert os.path.exists(path) == (not should_delete)
+
+
+def test_tempdir_cleanup_ignore_errors() -> None:
+    os_unlink = os.unlink
+
+    # mock os.unlink to fail with EACCES for a specific filename to simulate
+    # how removing a loaded exe/dll behaves.
+    def unlink(name: str, *args: Any, **kwargs: Any) -> None:
+        if "bomb" in name:
+            raise PermissionError(name)
+        else:
+            os_unlink(name)
+
+    with mock.patch("os.unlink", unlink):
+        with TempDirectory(ignore_cleanup_errors=True) as tmp_dir:
+            path = tmp_dir.path
+            with open(os.path.join(path, "bomb"), "a"):
+                pass
+
+    filename = os.path.join(path, "bomb")
+    assert os.path.isfile(filename)
+    os.unlink(filename)