Clean torn tool dirs and escalate total pre-extraction failure

esphome/espidf/install_tool_archives.py

@@ -4,13 +4,18 @@ Run via ``python <this file> <idf_framework_root> <targets-csv> <workers>
 <tool-spec>...`` 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()

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")
 
 

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(