Repository: pytest-dev/pytest
Pull request: Inaccessible lock files now imply temporary directories can't be removed
Fix #7491


Problem statement:
Cleaning up temporary directories occasionally raises PermissionError
On Windows, I'm finding that *pytest* occasionally raises an exception starting with `PermissionError: [WinError 5] Access is denied` while cleaning up its temporary directories.  Below is an example of the output of a test session in which the exception arises.  The test file contains only the function `test_temp` shown in the output.  A necessary condition for the exception is that *pytest*'s base temporary directory already contains at least three temporary directories to cause *pytest* to try to clean up at least one directory.  Also, the exception occurred more often when the computer was under load.

    ============================= test session starts =============================
    platform win32 -- Python 3.7.7, pytest-5.4.3, py-1.9.0, pluggy-0.13.1
    rootdir: C:\Users\stan.west\Desktop\pytest-garbage
    collected 1 item

    test_temp.py F                                                           [100%]

    ================================== FAILURES ===================================
    __________________________________ test_temp __________________________________

    tmp_path_factory = TempPathFactory(_given_basetemp=None, _trace=<pluggy._tracing.TagTracerSub object at 0x0000026E365FECC8>, _basetemp=None)

        def test_temp(tmp_path_factory):
            for _ in range(1000):
    >           tmp_path_factory.mktemp("temp")

    test_temp.py:3:
    _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
    ..\..\Programs\Miniconda3-64\envs\pytest-garbage\lib\site-packages\_pytest\tmpdir.py:71: in mktemp
        basename = self._ensure_relative_to_basetemp(basename)
    ..\..\Programs\Miniconda3-64\envs\pytest-garbage\lib\site-packages\_pytest\tmpdir.py:50: in _ensure_relative_to_basetemp
        if (self.getbasetemp() / basename).resolve().parent != self.getbasetemp():
    ..\..\Programs\Miniconda3-64\envs\pytest-garbage\lib\site-packages\_pytest\tmpdir.py:98: in getbasetemp
        prefix="pytest-", root=rootdir, keep=3, lock_timeout=LOCK_TIMEOUT
    ..\..\Programs\Miniconda3-64\envs\pytest-garbage\lib\site-packages\_pytest\pathlib.py:344: in make_numbered_dir_with_cleanup
        consider_lock_dead_if_created_before=consider_lock_dead_if_created_before,
    ..\..\Programs\Miniconda3-64\envs\pytest-garbage\lib\site-packages\_pytest\pathlib.py:323: in cleanup_numbered_dir
        try_cleanup(path, consider_lock_dead_if_created_before)
    ..\..\Programs\Miniconda3-64\envs\pytest-garbage\lib\site-packages\_pytest\pathlib.py:300: in try_cleanup
        if ensure_deletable(path, consider_lock_dead_if_created_before):
    ..\..\Programs\Miniconda3-64\envs\pytest-garbage\lib\site-packages\_pytest\pathlib.py:284: in ensure_deletable
        if not lock.exists():
    ..\..\Programs\Miniconda3-64\envs\pytest-garbage\lib\pathlib.py:1356: in exists
        self.stat()
    _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

    self = WindowsPath('C:/Users/stan.west/AppData/Local/Temp/pytest-of-stan.west/garbage-f1c50674-fd35-4f5b-b6c5-1ad95ba7ffa7/.lock')

        def stat(self):
            """
            Return the result of the stat() system call on this path, like
            os.stat() does.
            """
    >       return self._accessor.stat(self)
    E       PermissionError: [WinError 5] Access is denied: 'C:\\Users\\stan.west\\AppData\\Local\\Temp\\pytest-of-stan.west\\garbage-f1c50674-fd35-4f5b-b6c5-1ad95ba7ffa7\\.lock'

    ..\..\Programs\Miniconda3-64\envs\pytest-garbage\lib\pathlib.py:1178: PermissionError
    =========================== short test summary info ===========================
    FAILED test_temp.py::test_temp - PermissionError: [WinError 5] Access is deni...

    ============================== 1 failed in 0.83s ==============================

It seems that sometimes the operating system continued to actually delete the files and directories inside an old directory even after the `cleanup_numbered_dir` function (below) completed the call in its first `for` statement to `try_cleanup`.  Then, the second `for` statement found that lingering directory, which `try_cleanup` renamed to the form `garbage-*`.  While `try_cleanup` was attempting again to delete its contents, the operating system finished actually deleting them, and the exception occurred.

    def cleanup_numbered_dir(
        root: Path, prefix: str, keep: int, consider_lock_dead_if_created_before: float
    ) -> None:
        """cleanup for lock driven numbered directories"""
        for path in cleanup_candidates(root, prefix, keep):
            try_cleanup(path, consider_lock_dead_if_created_before)
        for path in root.glob("garbage-*"):
            try_cleanup(path, consider_lock_dead_if_created_before)

I tested simply reversing the two `for` statements, so that *pytest* cleans old `garbage-*` directories before numbered directories, and that appeared to prevent the exception in my testing.

The operating system is Windows 10.0.17134 Build 17134, the file system is NTFS on a solid-state drive, I'm using a *conda* environment, and `pip list` produces:

    Package            Version
    ------------------ -------------------
    atomicwrites       1.4.0
    attrs              19.3.0
    certifi            2020.6.20
    colorama           0.4.3
    importlib-metadata 1.7.0
    more-itertools     8.4.0
    packaging          20.4
    pip                20.1.1
    pluggy             0.13.1
    py                 1.9.0
    pyparsing          2.4.7
    pytest             5.4.3
    setuptools         47.3.1.post20200622
    six                1.15.0
    wcwidth            0.2.5
    wheel              0.34.2
    wincertstore       0.2
    zipp               3.1.0

I also encountered the same exception using *pytest* version 6.0.0rc1, although the session output differs because *pytest* defers the clean-up until exit:

    ============================= test session starts =============================
    platform win32 -- Python 3.7.7, pytest-6.0.0rc1, py-1.9.0, pluggy-0.13.1
    rootdir: C:\Users\stan.west\Desktop\pytest-garbage
    collected 1 item

    test_temp.py .                                                           [100%]

    ============================== 1 passed in 2.67s ==============================
    Error in atexit._run_exitfuncs:
    Traceback (most recent call last):
    File "c:\users\stan.west\programs\miniconda3-64\envs\pytest-garbage\lib\pathlib.py", line 1356, in exists
        self.stat()
    File "c:\users\stan.west\programs\miniconda3-64\envs\pytest-garbage\lib\pathlib.py", line 1178, in stat
        return self._accessor.stat(self)
    PermissionError: [WinError 5] Access is denied: 'C:\\Users\\stan.west\\AppData\\Local\\Temp\\pytest-of-stan.west\\garbage-02f6a08e-f05a-46d7-bd84-4a35962efb26\\.lock'

Is swapping the `for` statements within `cleanup_numbered_dir` a good way to resolve this issue?

Hints from issue discussion:
Hi @stanwest,

The order should not really matter in that situation, what might be happening is that by first cleaning up the `garbage-*` directories, this is giving more time for whatever is holding the locked directories to release that lock (say a background process), and then it might appear to solve the problem.

I believe the proper solution is to explicitly catch errors around that `exists` call, and assume it is locked in case of any errors.

Commit message: Inaccessible lock files imply temporary directories can't be removed

Fix #7491

Patch to review:
diff --git a/changelog/7491.bugfix.rst b/changelog/7491.bugfix.rst
new file mode 100644
index 00000000000..5b00a5713b5
--- /dev/null
+++ b/changelog/7491.bugfix.rst
@@ -0,0 +1,2 @@
+:fixture:`tmpdir` and :fixture:`tmp_path` no longer raise an error if the lock to check for
+stale temporary directories is not accessible.
diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py
index 6a0bf7f6f0e..1ca1f7f5f20 100644
--- a/src/_pytest/pathlib.py
+++ b/src/_pytest/pathlib.py
@@ -286,12 +286,17 @@ def maybe_delete_a_numbered_dir(path: Path) -> None:
 
 
 def ensure_deletable(path: Path, consider_lock_dead_if_created_before: float) -> bool:
-    """checks if a lock exists and breaks it if its considered dead"""
+    """checks if `path` is deletable based if the lock file is expired"""
     if path.is_symlink():
         return False
     lock = get_lock_path(path)
-    if not lock.exists():
-        return True
+    try:
+        if not lock.is_file():
+            return True
+    except OSError:
+        # we might not have access to the lock file at all, in this case assume
+        # we don't have access to the entire directory (#7491).
+        return False
     try:
         lock_time = lock.stat().st_mtime
     except Exception:
diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py
index d9d3894f935..834b5bb1589 100644
--- a/testing/test_pathlib.py
+++ b/testing/test_pathlib.py
@@ -358,7 +358,7 @@ def test_get_extended_length_path_str():
 
 
 def test_suppress_error_removing_lock(tmp_path):
-    """ensure_deletable should not raise an exception if the lock file cannot be removed (#5456)"""
+    """ensure_deletable should not be resilient if lock file cannot be removed (#5456, #7491)"""
     path = tmp_path / "dir"
     path.mkdir()
     lock = get_lock_path(path)
@@ -371,6 +371,12 @@ def test_suppress_error_removing_lock(tmp_path):
         )
     assert lock.is_file()
 
+    with unittest.mock.patch.object(Path, "is_file", side_effect=OSError):
+        assert not ensure_deletable(
+            path, consider_lock_dead_if_created_before=mtime + 30
+        )
+    assert lock.is_file()
+
     # check now that we can remove the lock file in normal circumstances
     assert ensure_deletable(path, consider_lock_dead_if_created_before=mtime + 30)
     assert not lock.is_file()


Code review comments:
diff_hunk: @@ -358,7 +358,7 @@ def test_get_extended_length_path_str():
 
 
 def test_suppress_error_removing_lock(tmp_path):
-    """ensure_deletable should not raise an exception if the lock file cannot be removed (#5456)"""
+    """ensure_deletable should not be resilient if lock file cannot be removed (#5456, #7491)"""
path: testing/test_pathlib.py
text: @user1:
```suggestion
    """ensure_deletable should be resilient if lock file cannot be removed (#5456, #7491)"""
```