From 2afd74ff099c2cb649a139ddbdfd6d93412789b5 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 27 Mar 2026 20:24:18 +0100 Subject: [PATCH 1/6] fix: rm_rf now handles directories without S_IXUSR permission (#7940) The on_rm_rf_error handler previously only set S_IRUSR|S_IWUSR (read+write) when retrying failed removals, and silently skipped os.open/os.scandir failures. This meant directories lacking execute permission (S_IXUSR) could not be traversed or removed by shutil.rmtree. Now the handler: - Uses S_IRWXU (read+write+execute) for all permission fixes - Handles os.open and os.scandir PermissionError by fixing permissions on the path and its parent, then removing the entry directly - Includes a guard against infinite recursion when chmod has no effect Supersedes #7941 which took the approach of delegating to tempfile.TemporaryDirectory._rmtree. Co-authored-by: Cursor AI Co-authored-by: Claude Opus --- changelog/7940.bugfix.rst | 1 + src/_pytest/pathlib.py | 59 +++++++++++++++++++++++++++++---------- testing/test_tmpdir.py | 58 +++++++++++++++++++++++++++++++++----- 3 files changed, 97 insertions(+), 21 deletions(-) create mode 100644 changelog/7940.bugfix.rst diff --git a/changelog/7940.bugfix.rst b/changelog/7940.bugfix.rst new file mode 100644 index 00000000000..ae0a7ad577e --- /dev/null +++ b/changelog/7940.bugfix.rst @@ -0,0 +1 @@ +Fixed ``rm_rf`` failing to remove directories when the ``S_IXUSR`` (owner execute) permission bit is missing -- supersedes :pr:`7941`. diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 291bdf4ecbf..7b4f47f83fe 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -69,6 +69,25 @@ def get_lock_path(path: _AnyPurePath) -> _AnyPurePath: return path.joinpath(".lock") +def _chmod_rwx(p: str) -> bool: + """Grant owner read, write, and execute permissions. + + Returns True if permissions were actually changed, False if they were + already sufficient or couldn't be changed. + """ + import stat + + try: + old_mode = os.stat(p).st_mode + new_mode = old_mode | stat.S_IRWXU + if old_mode == new_mode: + return False + os.chmod(p, new_mode) + except OSError: + return False + return True + + def on_rm_rf_error( func: Callable[..., Any] | None, path: str, @@ -97,32 +116,44 @@ def on_rm_rf_error( ) return False + if func in (os.open, os.scandir): + # Directory traversal failed (e.g. missing S_IXUSR). Fix permissions + # on the path and its parent, then remove it ourselves since rmtree + # skips entries after the error handler returns. + # See: https://github.com/pytest-dev/pytest/issues/7940 + p = Path(path) + if p.parent != p: + _chmod_rwx(str(p.parent)) + if not _chmod_rwx(path): + return False + if os.path.isdir(path): + rm_rf(Path(path)) + else: + try: + os.unlink(path) + except OSError: + return False + return True + if func not in (os.rmdir, os.remove, os.unlink): - if func not in (os.open,): - warnings.warn( - PytestWarning( - f"(rm_rf) unknown function {func} when removing {path}:\n{type(exc)}: {exc}" - ) + warnings.warn( + PytestWarning( + f"(rm_rf) unknown function {func} when removing {path}:\n{type(exc)}: {exc}" ) + ) return False # Chmod + retry. - import stat - - def chmod_rw(p: str) -> None: - mode = os.stat(p).st_mode - os.chmod(p, mode | stat.S_IRUSR | stat.S_IWUSR) - # For files, we need to recursively go upwards in the directories to - # ensure they all are also writable. + # ensure they all are also accessible and writable. p = Path(path) if p.is_file(): for parent in p.parents: - chmod_rw(str(parent)) + _chmod_rwx(str(parent)) # Stop when we reach the original path passed to rm_rf. if parent == start_path: break - chmod_rw(str(path)) + _chmod_rwx(str(path)) func(path) return True diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index 610de9c5fd6..d7d44e35a30 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -566,6 +566,25 @@ def test_rm_rf_with_read_only_directory(self, tmp_path): assert not adir.is_dir() + @pytest.mark.skipif(not hasattr(os, "getuid"), reason="unix permissions") + def test_rm_rf_with_no_exec_permission_directories(self, tmp_path): + """Ensure rm_rf can remove directories without S_IXUSR (#7940). + + This is the exact scenario from the original issue: nested directories + and files with all permissions stripped. + """ + p = tmp_path / "foo" / "bar" / "baz" + p.parent.mkdir(parents=True) + p.touch(mode=0) + for parent in p.parents: + if parent == tmp_path: + break + parent.chmod(mode=0) + + rm_rf(tmp_path / "foo") + + assert not (tmp_path / "foo").exists() + def test_on_rm_rf_error(self, tmp_path: Path) -> None: adir = tmp_path / "dir" adir.mkdir() @@ -593,17 +612,42 @@ def test_on_rm_rf_error(self, tmp_path: Path) -> None: on_rm_rf_error(None, str(fn), exc_info3, start_path=tmp_path) assert fn.is_file() - # ignored function - with warnings.catch_warnings(record=True) as w: - exc_info4 = PermissionError() - on_rm_rf_error(os.open, str(fn), exc_info4, start_path=tmp_path) - assert fn.is_file() - assert not [x.message for x in w] - + # os.unlink PermissionError is handled (chmod + retry) exc_info5 = PermissionError() on_rm_rf_error(os.unlink, str(fn), exc_info5, start_path=tmp_path) assert not fn.is_file() + def test_on_rm_rf_error_os_open_handles_file(self, tmp_path: Path) -> None: + """os.open PermissionError on a file is handled by fixing + permissions and removing it (#7940).""" + adir = tmp_path / "dir" + adir.mkdir() + fn = adir / "foo.txt" + fn.touch() + self.chmod_r(fn) + + with warnings.catch_warnings(record=True) as w: + exc_info = PermissionError() + on_rm_rf_error(os.open, str(fn), exc_info, start_path=tmp_path) + assert not fn.exists() + assert not [x.message for x in w] + + @pytest.mark.skipif(not hasattr(os, "getuid"), reason="unix permissions") + def test_on_rm_rf_error_os_open_handles_directory(self, tmp_path: Path) -> None: + """os.open PermissionError on a directory is handled by fixing + permissions and recursively removing it (#7940).""" + adir = tmp_path / "dir" + adir.mkdir() + (adir / "child").mkdir() + (adir / "child" / "file.txt").touch() + os.chmod(str(adir), 0o600) + + with warnings.catch_warnings(record=True) as w: + exc_info = PermissionError() + on_rm_rf_error(os.open, str(adir), exc_info, start_path=tmp_path) + assert not adir.exists() + assert not [x.message for x in w] + def attempt_symlink_to(path, to_path): """Try to make a symlink from "path" to "to_path", skipping in case this platform From ce059863bef949d5990c9f9cb353ae7e74b8ead9 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Fri, 27 Mar 2026 20:43:47 +0100 Subject: [PATCH 2/6] Address Copilot review feedback on rm_rf permission handling - Use stat.S_IMODE() to extract only permission bits before comparing/setting, avoiding file-type bit leakage into os.chmod - Only add S_IXUSR for directories; regular files get S_IRUSR|S_IWUSR only, avoiding unnecessary execute side-effect on files - Fix recursion guard: track whether *either* parent or path chmod changed permissions, so fixing the parent alone (the common case for missing S_IXUSR) is sufficient to proceed with removal - Add test for parent-only permission fix scenario Co-authored-by: Cursor AI Co-authored-by: Claude Opus --- src/_pytest/pathlib.py | 18 +++++++++++++----- testing/test_tmpdir.py | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 7b4f47f83fe..781f426a29d 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -70,7 +70,11 @@ def get_lock_path(path: _AnyPurePath) -> _AnyPurePath: def _chmod_rwx(p: str) -> bool: - """Grant owner read, write, and execute permissions. + """Grant owner sufficient permissions for deletion. + + Directories get ``S_IRWXU`` (read+write+exec for traversal). + Regular files get ``S_IRUSR | S_IWUSR`` only, to avoid making + non-executable files executable as a side effect. Returns True if permissions were actually changed, False if they were already sufficient or couldn't be changed. @@ -79,8 +83,10 @@ def _chmod_rwx(p: str) -> bool: try: old_mode = os.stat(p).st_mode - new_mode = old_mode | stat.S_IRWXU - if old_mode == new_mode: + perm_mode = stat.S_IMODE(old_mode) + bits = stat.S_IRWXU if stat.S_ISDIR(old_mode) else stat.S_IRUSR | stat.S_IWUSR + new_mode = perm_mode | bits + if perm_mode == new_mode: return False os.chmod(p, new_mode) except OSError: @@ -122,9 +128,11 @@ def on_rm_rf_error( # skips entries after the error handler returns. # See: https://github.com/pytest-dev/pytest/issues/7940 p = Path(path) + parent_changed = False if p.parent != p: - _chmod_rwx(str(p.parent)) - if not _chmod_rwx(path): + parent_changed = _chmod_rwx(str(p.parent)) + path_changed = _chmod_rwx(path) + if not (parent_changed or path_changed): return False if os.path.isdir(path): rm_rf(Path(path)) diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index d7d44e35a30..ef88c6fdc94 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -648,6 +648,26 @@ def test_on_rm_rf_error_os_open_handles_directory(self, tmp_path: Path) -> None: assert not adir.exists() assert not [x.message for x in w] + @pytest.mark.skipif(not hasattr(os, "getuid"), reason="unix permissions") + def test_on_rm_rf_error_os_open_parent_perms(self, tmp_path: Path) -> None: + """When the PermissionError is caused by the *parent* directory lacking + S_IXUSR, fixing the parent is sufficient even if the child already has + correct permissions.""" + parent = tmp_path / "parent" + parent.mkdir() + child = parent / "child" + child.mkdir() + (child / "file.txt").touch() + # Child has full perms, but parent lacks execute -> os.open(child) fails. + os.chmod(str(parent), 0o600) + + with warnings.catch_warnings(record=True) as w: + exc_info = PermissionError() + result = on_rm_rf_error(os.open, str(child), exc_info, start_path=tmp_path) + assert result is True + assert not child.exists() + assert not [x.message for x in w] + def attempt_symlink_to(path, to_path): """Try to make a symlink from "path" to "to_path", skipping in case this platform From 58485d78f4908331413a865709787a84af831001 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Sun, 24 May 2026 23:11:09 +0200 Subject: [PATCH 3/6] fix: address review feedback on rm_rf permission handling - Move `import stat` to module level (no lazy import) - Bound parent chmod by start_path to prevent escaping rm_rf scope - Use `p.is_dir()` and `rm_rf(p)` consistently with Path objects - Rewrite changelog to be user-facing (no internal function names) Co-authored-by: Cursor AI Co-authored-by: Anthropic Claude Sonnet 4 --- changelog/7940.bugfix.rst | 2 +- src/_pytest/pathlib.py | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/changelog/7940.bugfix.rst b/changelog/7940.bugfix.rst index ae0a7ad577e..99845153ace 100644 --- a/changelog/7940.bugfix.rst +++ b/changelog/7940.bugfix.rst @@ -1 +1 @@ -Fixed ``rm_rf`` failing to remove directories when the ``S_IXUSR`` (owner execute) permission bit is missing -- supersedes :pr:`7941`. +Fixed cleanup of temporary directories failing when subdirectories have their execute permission removed. diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 781f426a29d..e067f212db0 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -24,6 +24,7 @@ from pathlib import PurePath from posixpath import sep as posix_sep import shutil +import stat import sys import types from types import ModuleType @@ -79,8 +80,6 @@ def _chmod_rwx(p: str) -> bool: Returns True if permissions were actually changed, False if they were already sufficient or couldn't be changed. """ - import stat - try: old_mode = os.stat(p).st_mode perm_mode = stat.S_IMODE(old_mode) @@ -124,18 +123,19 @@ def on_rm_rf_error( if func in (os.open, os.scandir): # Directory traversal failed (e.g. missing S_IXUSR). Fix permissions - # on the path and its parent, then remove it ourselves since rmtree - # skips entries after the error handler returns. + # on the path and its parent (bounded by start_path), then remove it + # ourselves since rmtree skips entries after the error handler returns. # See: https://github.com/pytest-dev/pytest/issues/7940 p = Path(path) parent_changed = False - if p.parent != p: - parent_changed = _chmod_rwx(str(p.parent)) + parent = p.parent + if parent not in (p, start_path): + parent_changed = _chmod_rwx(str(parent)) path_changed = _chmod_rwx(path) if not (parent_changed or path_changed): return False - if os.path.isdir(path): - rm_rf(Path(path)) + if p.is_dir(): + rm_rf(p) else: try: os.unlink(path) From d9fd7040fab36afcf7abd9e601b319fb486412a9 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Mon, 25 May 2026 07:05:47 +0200 Subject: [PATCH 4/6] test: add coverage for _chmod_rwx OSError and recursion guard paths - Test _chmod_rwx returns False on nonexistent path (OSError branch) - Test _chmod_rwx returns False when permissions already sufficient - Test os.open handler returns False when chmod is ineffective Co-authored-by: Cursor AI Co-authored-by: Anthropic Claude Sonnet 4 --- testing/test_tmpdir.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index ef88c6fdc94..cc1bbb35c5f 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -14,6 +14,7 @@ from _pytest import pathlib from _pytest.config import Config from _pytest.monkeypatch import MonkeyPatch +from _pytest.pathlib import _chmod_rwx from _pytest.pathlib import cleanup_numbered_dir from _pytest.pathlib import create_cleanup_lock from _pytest.pathlib import make_numbered_dir @@ -668,6 +669,35 @@ def test_on_rm_rf_error_os_open_parent_perms(self, tmp_path: Path) -> None: assert not child.exists() assert not [x.message for x in w] + def test_chmod_rwx_returns_false_on_nonexistent_path(self, tmp_path: Path) -> None: + """_chmod_rwx returns False when the path doesn't exist (OSError).""" + nonexistent = tmp_path / "does_not_exist" + assert _chmod_rwx(str(nonexistent)) is False + + def test_chmod_rwx_returns_false_when_already_sufficient( + self, tmp_path: Path + ) -> None: + """_chmod_rwx returns False when permissions are already sufficient.""" + d = tmp_path / "dir" + d.mkdir(mode=0o700) + assert _chmod_rwx(str(d)) is False + + f = tmp_path / "file" + f.touch(mode=0o600) + assert _chmod_rwx(str(f)) is False + + def test_on_rm_rf_error_os_open_returns_false_when_chmod_ineffective( + self, tmp_path: Path + ) -> None: + """os.open handler returns False when neither parent nor path chmod + changes anything (recursion guard).""" + adir = tmp_path / "dir" + adir.mkdir(mode=0o700) + exc_info = PermissionError() + result = on_rm_rf_error(os.open, str(adir), exc_info, start_path=tmp_path) + assert result is False + assert adir.exists() + def attempt_symlink_to(path, to_path): """Try to make a symlink from "path" to "to_path", skipping in case this platform From dfc683446cf669a500d914e52efb7a9022220da1 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Mon, 25 May 2026 10:17:30 +0200 Subject: [PATCH 5/6] test: add coverage for unlink OSError and parent walk, add no-branch pragmas - Test os.open handler returns False when unlink raises OSError - Test os.unlink handler walks multiple read-only parent directories - Add pragma: no branch to for-loops that always exit via break Co-authored-by: Cursor AI Co-authored-by: Anthropic Claude Sonnet 4 --- src/_pytest/pathlib.py | 2 +- testing/test_tmpdir.py | 40 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index e067f212db0..2fd19183479 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -156,7 +156,7 @@ def on_rm_rf_error( # ensure they all are also accessible and writable. p = Path(path) if p.is_file(): - for parent in p.parents: + for parent in p.parents: # pragma: no branch _chmod_rwx(str(parent)) # Stop when we reach the original path passed to rm_rf. if parent == start_path: diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index cc1bbb35c5f..717fda99e4c 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -523,6 +523,10 @@ def test_removal_accepts_lock(self, tmp_path): assert dir.is_dir() +def _raise_oserror(*args: object, **kwargs: object) -> None: + raise OSError("simulated failure") + + class TestRmRf: def test_rm_rf(self, tmp_path): adir = tmp_path / "adir" @@ -577,7 +581,7 @@ def test_rm_rf_with_no_exec_permission_directories(self, tmp_path): p = tmp_path / "foo" / "bar" / "baz" p.parent.mkdir(parents=True) p.touch(mode=0) - for parent in p.parents: + for parent in p.parents: # pragma: no branch if parent == tmp_path: break parent.chmod(mode=0) @@ -698,6 +702,40 @@ def test_on_rm_rf_error_os_open_returns_false_when_chmod_ineffective( assert result is False assert adir.exists() + def test_on_rm_rf_error_os_open_unlink_fails( + self, tmp_path: Path, monkeypatch: MonkeyPatch + ) -> None: + """os.open handler returns False when chmod succeeds but os.unlink + still raises OSError (e.g. sandbox or other mechanism).""" + fn = tmp_path / "stubborn.txt" + fn.touch(mode=0) + + monkeypatch.setattr(os, "unlink", _raise_oserror) + + exc_info = PermissionError() + result = on_rm_rf_error(os.open, str(fn), exc_info, start_path=tmp_path) + assert result is False + assert fn.exists() + + @pytest.mark.skipif(not hasattr(os, "getuid"), reason="unix permissions") + def test_on_rm_rf_error_chmod_retry_walks_parents(self, tmp_path: Path) -> None: + """The os.rmdir/os.unlink handler walks up through multiple parent + directories to fix permissions before retrying.""" + deep = tmp_path / "a" / "b" / "c" + deep.mkdir(parents=True) + fn = deep / "file.txt" + fn.touch() + # Remove write from intermediate dirs (keep exec so traversal works, + # but os.unlink needs write on the parent). + for parent in fn.parents: # pragma: no branch + if parent == tmp_path: + break + parent.chmod(0o555) + + exc_info = PermissionError() + on_rm_rf_error(os.unlink, str(fn), exc_info, start_path=tmp_path) + assert not fn.exists() + def attempt_symlink_to(path, to_path): """Try to make a symlink from "path" to "to_path", skipping in case this platform From 9511d93b73b749aeea081c1f2b3a2e393d5957b8 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Mon, 25 May 2026 14:53:27 +0200 Subject: [PATCH 6/6] refactor: hoist p = Path(path) above branches in on_rm_rf_error Move the Path conversion to a single location shared by both the os.open/os.scandir handler and the os.rmdir/os.unlink handler, as suggested in review. Co-authored-by: Cursor AI Co-authored-by: Anthropic Claude Sonnet 4 --- src/_pytest/pathlib.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 2fd19183479..8258e64cb1d 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -121,12 +121,13 @@ def on_rm_rf_error( ) return False + p = Path(path) + if func in (os.open, os.scandir): # Directory traversal failed (e.g. missing S_IXUSR). Fix permissions # on the path and its parent (bounded by start_path), then remove it # ourselves since rmtree skips entries after the error handler returns. # See: https://github.com/pytest-dev/pytest/issues/7940 - p = Path(path) parent_changed = False parent = p.parent if parent not in (p, start_path): @@ -154,7 +155,6 @@ def on_rm_rf_error( # Chmod + retry. # For files, we need to recursively go upwards in the directories to # ensure they all are also accessible and writable. - p = Path(path) if p.is_file(): for parent in p.parents: # pragma: no branch _chmod_rwx(str(parent))