From 8c91612977e9f440745183556e573e30c28bd6da Mon Sep 17 00:00:00 2001 From: Jawaun Brown Date: Tue, 25 Aug 2026 19:06:56 -0400 Subject: [PATCH 1/2] pathlib: use PID liveness as primary stale-lock signal `ensure_deletable` previously treated a `.lock` file as dead purely by mtime age, with LOCK_TIMEOUT = 3 days. The PID inside the lock was never consulted. Two failure modes followed: - A crashed session's scratch survives for three days before it can be reaped, even though the process is obviously gone. - A long-running session (some suites do run for hours) whose lock happens to be older than three days can have its own scratch reaped mid-run. This change reads the PID out of the lock file and probes it with `os.kill(pid, 0)` on POSIX and `OpenProcess` on Windows. If the PID is provably not running, the lock is unlinked and the directory is reported deletable regardless of clock. If the PID is alive, the directory is not deletable regardless of clock. The mtime-based check remains as a fallback for locks whose contents cannot be parsed (empty, corrupted, or partially written). Refs #14935. --- changelog/14935.improvement.2.rst | 7 ++ src/_pytest/pathlib.py | 118 +++++++++++++++++++++++++++--- testing/test_tmpdir.py | 46 +++++++++++- 3 files changed, 159 insertions(+), 12 deletions(-) create mode 100644 changelog/14935.improvement.2.rst diff --git a/changelog/14935.improvement.2.rst b/changelog/14935.improvement.2.rst new file mode 100644 index 00000000000..3ead8fae196 --- /dev/null +++ b/changelog/14935.improvement.2.rst @@ -0,0 +1,7 @@ +The stale-``.lock`` check in ``ensure_deletable`` now uses PID liveness +as its primary signal, with the historical ``LOCK_TIMEOUT`` (three days) +retained as a fallback for locks whose contents cannot be parsed. A +lock whose owner process is provably not running is reaped immediately +instead of surviving for three days, and a lock whose owner is still +running is respected regardless of its ``mtime``, so a long-running +session can no longer have its own scratch reaped mid-run. diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 8eb593d194d..56a55deae50 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -348,8 +348,90 @@ def maybe_delete_a_numbered_dir(path: Path) -> None: pass +def _pid_alive(pid: int) -> bool: + """Return True if a process with the given PID currently exists. + + Used by :func:`ensure_deletable` to determine whether a lock's owner is + still running, so that stale-lock detection does not depend solely on + wall-clock age. + + Returns True when we cannot tell (permission errors, unsupported + platform behaviour) so that ambiguous cases fall through to the + mtime-based check rather than eagerly deleting. + """ + if pid <= 0: + return False + if sys.platform == "win32": + # OpenProcess with PROCESS_QUERY_LIMITED_INFORMATION; if the handle + # opens the process still exists in some form. If it doesn't, the + # process is gone. + try: + import ctypes + from ctypes import wintypes + + PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 + STILL_ACTIVE = 259 + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + handle = kernel32.OpenProcess( + PROCESS_QUERY_LIMITED_INFORMATION, False, pid + ) + if not handle: + # ERROR_INVALID_PARAMETER (87) means "no such process". + if ctypes.get_last_error() == 87: + return False + # Any other failure: be conservative, assume alive. + return True + try: + exit_code = wintypes.DWORD() + if not kernel32.GetExitCodeProcess( + handle, ctypes.byref(exit_code) + ): + return True + return exit_code.value == STILL_ACTIVE + finally: + kernel32.CloseHandle(handle) + except Exception: + return True + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + # Process exists but is owned by another user; still alive. + return True + except OSError: + # Unknown; be conservative. + return True + return True + + +def _read_lock_pid(lock: Path) -> int | None: + """Return the PID stored in a lock file, or None if it can't be read.""" + try: + raw = lock.read_bytes() + except OSError: + return None + try: + return int(raw.strip()) + except (ValueError, UnicodeDecodeError): + return None + + def ensure_deletable(path: Path, consider_lock_dead_if_created_before: float) -> bool: - """Check if `path` is deletable based on whether the lock file is expired.""" + """Check if ``path`` is deletable. + + A directory is deletable when either: + + - it has no ``.lock`` file, or + - the ``.lock`` file names a PID that is provably not running, or + - the ``.lock`` file's mtime is older than + ``consider_lock_dead_if_created_before`` (legacy fallback for locks + whose contents can't be read). + + The PID check is the primary signal so that a dead session's lock is + reaped immediately, and a live long-running session's lock is respected + regardless of wall-clock age. + """ if path.is_symlink(): return False lock = get_lock_path(path) @@ -360,20 +442,34 @@ def ensure_deletable(path: Path, consider_lock_dead_if_created_before: float) -> # 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 + + pid = _read_lock_pid(lock) + if pid is not None: + if _pid_alive(pid): + # Owner is still running — respect the lock, regardless of age. + return False + # Owner is provably gone. Unlink the lock and let the caller delete + # the directory. If unlink races with another cleanup we're still + # correct: the directory is either already gone, or the next call + # will pick it up. + with contextlib.suppress(OSError): + lock.unlink() + return True + + # Couldn't parse the lock (empty, corrupt, or unreadable): fall through + # to the historical mtime-based check as a safety net. try: lock_time = lock.stat().st_mtime except Exception: return False - else: - if lock_time < consider_lock_dead_if_created_before: - # We want to ignore any errors while trying to remove the lock such as: - # - PermissionDenied, like the file permissions have changed since the lock creation; - # - FileNotFoundError, in case another pytest process got here first; - # and any other cause of failure. - with contextlib.suppress(OSError): - lock.unlink() - return True - return False + if lock_time < consider_lock_dead_if_created_before: + # See original comment: swallow any error unlinking the lock — it + # may have already been removed by a concurrent cleanup, or its + # permissions may have changed since creation. + with contextlib.suppress(OSError): + lock.unlink() + return True + return False def try_cleanup(path: Path, consider_lock_dead_if_created_before: float) -> None: diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index 0b33a74b926..a05055231f1 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -504,13 +504,57 @@ def test_cleanup_locked(self, tmp_path): create_cleanup_lock(p) + # Lock names the current PID, so ensure_deletable must return False + # regardless of the mtime threshold — pid-liveness is the primary + # signal, and the current process is alive. assert not pathlib.ensure_deletable( p, consider_lock_dead_if_created_before=p.stat().st_mtime - 1 ) - assert pathlib.ensure_deletable( + assert not pathlib.ensure_deletable( p, consider_lock_dead_if_created_before=p.stat().st_mtime + 1 ) + def test_cleanup_dead_pid_deletable_regardless_of_mtime( + self, tmp_path + ) -> None: + """A lock naming a PID that is provably not running is deletable + immediately, without waiting for the 3-day LOCK_TIMEOUT.""" + p = make_numbered_dir(root=tmp_path, prefix=self.PREFIX) + lock = p / ".lock" + # Pick a PID very unlikely to exist. Fall back to trying successive + # PIDs if by bad luck the first one is live. + dead_pid = 2**30 + while True: + try: + os.kill(dead_pid, 0) + except (ProcessLookupError, OSError): + break + dead_pid += 1 + lock.write_bytes(str(dead_pid).encode()) + # Pass a threshold in the future — mtime path would say "not old + # enough". With the new pid check, dead pid ⇒ deletable anyway. + assert pathlib.ensure_deletable( + p, consider_lock_dead_if_created_before=lock.stat().st_mtime + 10_000 + ) + assert not lock.exists() + + def test_cleanup_unreadable_lock_falls_back_to_mtime( + self, tmp_path + ) -> None: + """If the lock contents can't be parsed as a PID, ensure_deletable + falls back to the historical mtime-based check.""" + p = make_numbered_dir(root=tmp_path, prefix=self.PREFIX) + lock = p / ".lock" + lock.write_bytes(b"not-a-pid") + # mtime not yet past threshold → not deletable. + assert not pathlib.ensure_deletable( + p, consider_lock_dead_if_created_before=lock.stat().st_mtime - 1 + ) + # mtime past threshold → deletable via legacy path. + assert pathlib.ensure_deletable( + p, consider_lock_dead_if_created_before=lock.stat().st_mtime + 1 + ) + def test_cleanup_ignores_symlink(self, tmp_path): the_symlink = tmp_path / (self.PREFIX + "current") attempt_symlink_to(the_symlink, tmp_path / (self.PREFIX + "5")) From 93678d6e52fb04870e4d60d2267f59973f127b28 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 25 Aug 2026 23:07:44 +0000 Subject: [PATCH 2/2] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/_pytest/pathlib.py | 8 ++------ testing/test_tmpdir.py | 8 ++------ 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 56a55deae50..eda37a0ba8e 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -372,9 +372,7 @@ def _pid_alive(pid: int) -> bool: PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 STILL_ACTIVE = 259 kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) - handle = kernel32.OpenProcess( - PROCESS_QUERY_LIMITED_INFORMATION, False, pid - ) + handle = kernel32.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, False, pid) if not handle: # ERROR_INVALID_PARAMETER (87) means "no such process". if ctypes.get_last_error() == 87: @@ -383,9 +381,7 @@ def _pid_alive(pid: int) -> bool: return True try: exit_code = wintypes.DWORD() - if not kernel32.GetExitCodeProcess( - handle, ctypes.byref(exit_code) - ): + if not kernel32.GetExitCodeProcess(handle, ctypes.byref(exit_code)): return True return exit_code.value == STILL_ACTIVE finally: diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index a05055231f1..9f97d565fd8 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -514,9 +514,7 @@ def test_cleanup_locked(self, tmp_path): p, consider_lock_dead_if_created_before=p.stat().st_mtime + 1 ) - def test_cleanup_dead_pid_deletable_regardless_of_mtime( - self, tmp_path - ) -> None: + def test_cleanup_dead_pid_deletable_regardless_of_mtime(self, tmp_path) -> None: """A lock naming a PID that is provably not running is deletable immediately, without waiting for the 3-day LOCK_TIMEOUT.""" p = make_numbered_dir(root=tmp_path, prefix=self.PREFIX) @@ -538,9 +536,7 @@ def test_cleanup_dead_pid_deletable_regardless_of_mtime( ) assert not lock.exists() - def test_cleanup_unreadable_lock_falls_back_to_mtime( - self, tmp_path - ) -> None: + def test_cleanup_unreadable_lock_falls_back_to_mtime(self, tmp_path) -> None: """If the lock contents can't be parsed as a PID, ensure_deletable falls back to the historical mtime-based check.""" p = make_numbered_dir(root=tmp_path, prefix=self.PREFIX)