From 9ea429f0ef794e3b096d839d18a84862ea7a5af5 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 27 Aug 2026 18:06:41 -0500 Subject: [PATCH] Clean torn tool dirs and escalate total pre-extraction failure --- esphome/espidf/install_tool_archives.py | 43 +++++++++++++++++-- .../fixtures/idf_tools_stub/idf_tools.py | 13 ++++-- tests/unit_tests/test_espidf_framework.py | 30 +++++++++++-- 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/esphome/espidf/install_tool_archives.py b/esphome/espidf/install_tool_archives.py index 315a1ee4a9..fdc24540dc 100644 --- a/esphome/espidf/install_tool_archives.py +++ b/esphome/espidf/install_tool_archives.py @@ -4,13 +4,18 @@ Run via ``python ...`` with idf_tools on PYTHONPATH and IDF_TOOLS_PATH set. Drives idf_tools' own ``IDFTool.install()`` so extraction semantics match the sequential installer, which still runs afterwards as the authority and -redoes anything this best-effort pass failed on. +redoes anything this best-effort pass failed on. Archives are trusted from +the prefetch's sha256 verification, not re-hashed here. """ # pylint: disable=import-error # idf_tools is on PYTHONPATH at runtime only from concurrent.futures import ThreadPoolExecutor +from contextlib import suppress +import os from pathlib import Path +import shutil +import stat import sys from _tool_resolution import archive_name, init_idf_tools, iter_tool_downloads @@ -41,15 +46,38 @@ def collect_pending( return pending -def install_one(tool: object, name: str, version: str) -> None: +def _rmtree(path: str) -> None: + """Best-effort removal; clears the read-only bits that block deletion on + Windows (esphome.helpers.rmtree is not importable here).""" + + def _onexc(func, p, exc): # pragma: no cover # Windows read-only files + if os.access(p, os.W_OK): + raise exc + # Preserve the mode: 0o600 would strip a directory's execute bit + # and make the installer's own rmtree fail on the survivor + Path(p).chmod(Path(p).stat().st_mode | stat.S_IWUSR) + func(p) + + with suppress(OSError): + shutil.rmtree(path, onexc=_onexc) + if Path(path).exists(): # pragma: no cover + # A surviving torn dir may pass the installer's binary probe + print(f"could not remove {path}", file=sys.stderr) + + +def install_one(tool: object, name: str, version: str) -> bool: try: tool.install(version) # check_binary_valid exits via SystemExit; the installer redoes failures except (Exception, SystemExit) as e: # noqa: BLE001 # pylint: disable=broad-exception-caught + # A torn dest dir must not look installed to the installer + _rmtree(tool.get_path_for_version(version)) print( f"pre-extracting {name}@{version} failed, leaving it to the installer: {e}", file=sys.stderr, ) + return False + return True def main() -> None: @@ -67,8 +95,17 @@ def main() -> None: flush=True, ) with ThreadPoolExecutor(max_workers=workers) as ex: - for (name, version), tool in pending.items(): + futures = [ ex.submit(install_one, tool, name, version) + for (name, version), tool in pending.items() + ] + # Every job failing is a systematic fault; a nonzero exit makes the + # caller log it instead of silently degrading to a sequential install + failed = sum(not future.result() for future in futures) + if failed: + print(f"{failed} of {len(futures)} pre-extractions failed", file=sys.stderr) + if failed == len(futures): + sys.exit(1) main() diff --git a/tests/unit_tests/fixtures/idf_tools_stub/idf_tools.py b/tests/unit_tests/fixtures/idf_tools_stub/idf_tools.py index c24aa21013..28aefdd5b8 100644 --- a/tests/unit_tests/fixtures/idf_tools_stub/idf_tools.py +++ b/tests/unit_tests/fixtures/idf_tools_stub/idf_tools.py @@ -71,12 +71,17 @@ class _Tool: if self._broken: raise ToolBinaryError("broken binary") + def get_path_for_version(self, version: str) -> str: + return str(pathlib.Path(g.idf_tools_path) / "tools" / self.name / version) + def install(self, version: str) -> None: - # Real idf_tools' check_binary_valid failure path exits the process - if os.environ.get("TEST_FAIL_INSTALL") == self.name: - raise SystemExit(1) - dest = pathlib.Path(g.idf_tools_path) / "tools" / self.name / version + dest = pathlib.Path(self.get_path_for_version(version)) dest.mkdir(exist_ok=True, parents=True) + if self.name in os.environ.get("TEST_FAIL_INSTALL", "").split(","): + # Fail mid-install like a torn unpack: the partial dir is left + # behind and check_binary_valid's failure path exits the process + (dest / ".partial").write_text("torn", encoding="utf-8") + raise SystemExit(1) (dest / ".installed").write_text("ok", encoding="utf-8") diff --git a/tests/unit_tests/test_espidf_framework.py b/tests/unit_tests/test_espidf_framework.py index df146e019f..267a96294b 100644 --- a/tests/unit_tests/test_espidf_framework.py +++ b/tests/unit_tests/test_espidf_framework.py @@ -2136,8 +2136,8 @@ def test_install_tool_archives_failed_install_left_to_installer( monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], ) -> None: - """A per-tool failure (SystemExit from the binary check) warns and moves - on; the other tools still install.""" + """A per-tool failure warns, removes the torn dest dir so the installer + cannot trust it, and moves on; the other tools still install.""" _make_dist(tmp_path, "cmake.tar.gz", "ninja-v1.zip") monkeypatch.setenv("TEST_FAIL_INSTALL", "ninja") _run_espidf_script_inprocess( @@ -2145,8 +2145,30 @@ def test_install_tool_archives_failed_install_left_to_installer( ) tools = tmp_path / "tp" / "tools" assert (tools / "cmake" / "3.30.2" / ".installed").is_file() - assert not (tools / "ninja").exists() - assert "pre-extracting ninja@1.12.1 failed" in capsys.readouterr().err + assert not (tools / "ninja" / "1.12.1").exists() + err = capsys.readouterr().err + assert "pre-extracting ninja@1.12.1 failed" in err + assert "1 of 2 pre-extractions failed" in err + + +def test_install_tool_archives_all_failed_exits_nonzero( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """Every job failing is a systematic fault; the nonzero exit lets the + caller log it.""" + _make_dist(tmp_path, "cmake.tar.gz", "ninja-v1.zip") + monkeypatch.setenv("TEST_FAIL_INSTALL", "cmake,ninja") + with pytest.raises(SystemExit) as excinfo: + _run_espidf_script_inprocess( + tmp_path, monkeypatch, "install_tool_archives.py", "esp32", "4", "required" + ) + assert excinfo.value.code == 1 + assert "2 of 2 pre-extractions failed" in capsys.readouterr().err + tools = tmp_path / "tp" / "tools" + assert not (tools / "cmake" / "3.30.2").exists() + assert not (tools / "ninja" / "1.12.1").exists() def test_install_tool_archives_inprocess_dedupes_and_skips(