From cc41fd0beb6b6651ca2f46185409a98e75e694a4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 28 Aug 2026 13:42:42 -0500 Subject: [PATCH] [core] Make rmtree tolerate missing paths and concurrent directory changes (#18846) --- esphome/helpers.py | 48 +++++++++++++++++---- tests/unit_tests/test_helpers.py | 74 +++++++++++++++++++++++++++++++- 2 files changed, 113 insertions(+), 9 deletions(-) diff --git a/esphome/helpers.py b/esphome/helpers.py index a38fcaf821..3ccf0fe65a 100644 --- a/esphome/helpers.py +++ b/esphome/helpers.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Iterable, MutableMapping +from collections.abc import Callable, Iterable, MutableMapping from contextlib import suppress import ipaddress import logging @@ -456,23 +456,55 @@ def add_git_ceiling_directory(env: MutableMapping[str, str], directory: Path) -> env["GIT_CEILING_DIRECTORIES"] = os.pathsep.join(parts) -def rmtree(path: Path | str) -> None: - """Remove a directory tree, handling read-only files on Windows. +# Deletion attempts when a directory keeps being repopulated mid-delete +RMTREE_MAX_ATTEMPTS = 3 - On Windows, git pack files and other files may be marked read-only, - causing shutil.rmtree to fail. This handles that by removing the - read-only flag and retrying. + +def rmtree(path: Path | str) -> None: + """Remove a directory tree, tolerating common filesystem races. + + Read-only files (e.g. git pack files on Windows) get the read-only flag + removed and are retried. Paths that are already gone, whether the target + itself or entries vanishing mid-delete, are treated as removed. + Directories repopulated mid-delete (e.g. Finder recreating .DS_Store on + macOS) are retried a few times. """ + import errno import shutil + import time - def _onexc(func, path, exc): + def _onexc(func: Callable[..., object], path: str | Path, exc: OSError) -> None: + if isinstance(exc, FileNotFoundError): + _LOGGER.debug("rmtree: %s already gone", path) + return if os.access(path, os.W_OK): raise exc Path(path).chmod(stat.S_IWUSR | stat.S_IRUSR) func(path) - shutil.rmtree(path, onexc=_onexc) + last_err: OSError | None = None + for attempt in range(RMTREE_MAX_ATTEMPTS - 1): + try: + shutil.rmtree(path, onexc=_onexc) + return + except OSError as err: + if err.errno not in (errno.ENOTEMPTY, errno.EEXIST): + raise + _LOGGER.debug( + "rmtree: %s repopulated mid-delete (attempt %d): %s", + path, + attempt + 1, + err, + ) + last_err = err + # Give the racing writer (e.g. Finder) time to settle + time.sleep(0.05 * (attempt + 1)) + try: + shutil.rmtree(path, onexc=_onexc) + except OSError as err: + # Keep the earlier races visible in the traceback + raise err from last_err def walk_files(path: Path): diff --git a/tests/unit_tests/test_helpers.py b/tests/unit_tests/test_helpers.py index 53c326e0d0..ff82fa3c80 100644 --- a/tests/unit_tests/test_helpers.py +++ b/tests/unit_tests/test_helpers.py @@ -1,3 +1,4 @@ +import errno import io import logging import os @@ -5,7 +6,7 @@ from pathlib import Path import socket import stat import types -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch from aioesphomeapi.host_resolver import AddrInfo, IPv4Sockaddr, IPv6Sockaddr from hypothesis import given, settings @@ -966,6 +967,77 @@ def test_copy_file_if_changed_nonexistent_source(tmp_path: Path) -> None: helpers.copy_file_if_changed(src, dst) +def test_rmtree_removes_tree(tmp_path: Path) -> None: + """Test rmtree removes a populated directory tree.""" + target = tmp_path / "target" + (target / "sub").mkdir(parents=True) + (target / "sub" / "file.txt").write_text("content") + + helpers.rmtree(target) + assert not target.exists() + + +def test_rmtree_nonexistent_path(tmp_path: Path) -> None: + """Test rmtree on an already-removed path is a no-op.""" + helpers.rmtree(tmp_path / "gone") + + +def test_rmtree_retries_when_directory_repopulated(tmp_path: Path) -> None: + """Test rmtree retries when a file appears mid-delete (Finder .DS_Store race).""" + target = tmp_path / "target" + (target / "sub").mkdir(parents=True) + real_rmdir = os.rmdir + repopulated = False + + def racy_rmdir(path, **kwargs): + nonlocal repopulated + if not repopulated and Path(path).name == "target": + repopulated = True + (target / ".DS_Store").write_text("x") # Finder wins the race + real_rmdir(path, **kwargs) + + with patch("os.rmdir", side_effect=racy_rmdir), patch("time.sleep"): + helpers.rmtree(target) + assert repopulated + assert not target.exists() + + +def test_rmtree_raises_after_retries_exhausted(tmp_path: Path) -> None: + """Test rmtree gives up on a persistent ENOTEMPTY once attempts run out.""" + target = tmp_path / "target" + target.mkdir() + errs = [ + OSError(errno.ENOTEMPTY, "Directory not empty", str(target)) + for _ in range(helpers.RMTREE_MAX_ATTEMPTS) + ] + + with ( + patch("shutil.rmtree", side_effect=errs) as mock_rmtree, + patch("time.sleep") as mock_sleep, + pytest.raises(OSError, match="Directory not empty") as excinfo, + ): + helpers.rmtree(target) + assert mock_rmtree.call_count == helpers.RMTREE_MAX_ATTEMPTS + assert mock_sleep.call_args_list == [call(0.05), call(0.1)] + # Final failure chains to the last retried race + assert excinfo.value is errs[-1] + assert excinfo.value.__cause__ is errs[-2] + + +def test_rmtree_does_not_retry_other_oserror(tmp_path: Path) -> None: + """Test rmtree raises non-ENOTEMPTY errors immediately.""" + target = tmp_path / "target" + target.mkdir() + err = OSError(errno.EACCES, "Permission denied", str(target)) + + with ( + patch("shutil.rmtree", side_effect=err) as mock_rmtree, + pytest.raises(OSError, match="Permission denied"), + ): + helpers.rmtree(target) + assert mock_rmtree.call_count == 1 + + def test_resolve_ip_address_sorting() -> None: """Test that results are sorted by preference.""" # Create multiple address infos with different preferences