Verify pre-existing tool archives and escalate surviving torn dirs

esphome/espidf/framework.py

@@ -306,11 +306,19 @@ def _run_idf_tools_script(
         str(idf_framework_root),
         *(args or []),
     ]
+    # Explicit paths: the scripts dir (sibling imports must survive
+    # PYTHONSAFEPATH), the esphome package root, and the framework's idf_tools
+    pythonpath = os.pathsep.join(
+        (
+            str(_SCRIPTS_DIR),
+            str(_SCRIPTS_DIR.parents[1]),
+            str(Path(idf_framework_root) / "tools"),
+        )
+    )
     return run_command(
         cmd,
         msg=msg,
-        env=(env or os.environ)
-        | {"PYTHONPATH": str(Path(idf_framework_root) / "tools")},
+        env=(env or os.environ) | {"PYTHONPATH": pythonpath},
         stream_output=stream_output,
     )
 
@@ -729,9 +737,9 @@ def _prefetch_idf_tool_archives(
         dist_path = get_idf_tools_path() / "dist"
         entries = []
         seen_dests: set[str] = set()
+        # Pre-existing archives are not skipped: download_with_resume keeps
+        # them only on a sha256 match, so the pre-extraction can trust dist/
         for entry in json.loads(stdout):
-            if (dist_path / entry["dest"]).is_file():
-                continue
             # Never download unverified: an entry without sha256/size is
             # left to the installer, which fails loudly on a bad archive.
             # Checked before the dedupe so it cannot shadow a verifiable

esphome/espidf/install_tool_archives.py

@@ -1,7 +1,8 @@
 """Extract prefetched ESP-IDF tool archives in parallel.
 
 Run via ``python <this file> <idf_framework_root> <targets-csv> <workers>
-<tool-spec>...`` with idf_tools on PYTHONPATH and IDF_TOOLS_PATH set.
+<tool-spec>...`` with idf_tools and the esphome package root 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. Archives are trusted from
@@ -11,16 +12,14 @@ 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
 from idf_tools import ToolBinaryError, g
 
+from esphome.helpers import rmtree
+
 
 def collect_pending(
     targets_csv: str, tool_specs: list[str]
@@ -46,36 +45,26 @@ def collect_pending(
     return pending
 
 
-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:
+def install_one(tool: object, name: str, version: str) -> bool | None:
+    """True on success, False on a cleaned-up failure, None when the torn
+    dest dir survived and could fool the installer's binary probe."""
     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,
         )
+        # A torn dest dir must not look installed to the installer
+        dest = tool.get_path_for_version(version)
+        try:
+            rmtree(dest)
+        except FileNotFoundError:  # pragma: no cover  # failed before mkdir
+            pass
+        except OSError as cleanup_err:
+            print(f"could not remove {dest}: {cleanup_err}", file=sys.stderr)
+            return None
         return False
     return True
 
@@ -99,12 +88,13 @@ def main() -> None:
             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)
+    # A survivor could fool the installer; every job failing is systematic.
+    # Either way a nonzero exit makes the caller warn
+    results = [future.result() for future in futures]
+    failed = sum(result is not True for result in results)
     if failed:
-        print(f"{failed} of {len(futures)} pre-extractions failed", file=sys.stderr)
-        if failed == len(futures):
+        print(f"{failed} of {len(results)} pre-extractions failed", file=sys.stderr)
+        if None in results or failed == len(results):
             sys.exit(1)
 
 

tests/unit_tests/test_espidf_framework.py

@@ -1026,7 +1026,9 @@ def test_prefetch_downloads_archives_concurrently(tmp_path: Path) -> None:
     assert download.call_count == 6
 
 
-def test_prefetch_skips_already_downloaded_archives(tmp_path: Path) -> None:
+def test_prefetch_reverifies_already_downloaded_archives(tmp_path: Path) -> None:
+    """A pre-existing archive is not skipped: download_with_resume keeps it
+    only when the sha256 matches, so the pre-extraction can trust it."""
     dist = get_idf_tools_path() / "dist"
     dist.mkdir(parents=True)
     (dist / "cmake-3.30.2.tar.gz").write_bytes(b"cached")
@@ -1040,9 +1042,10 @@ def test_prefetch_skips_already_downloaded_archives(tmp_path: Path) -> None:
     ):
         _prefetch_idf_tool_archives(tmp_path, "esp32", ["required"], None)
 
-    # only the missing archive is downloaded
-    assert download.call_count == 1
-    assert download.call_args[0][1] == dist / "ninja.zip"
+    assert sorted(call[0][1] for call in download.call_args_list) == [
+        dist / "cmake-3.30.2.tar.gz",
+        dist / "ninja.zip",
+    ]
 
 
 @pytest.mark.parametrize(
@@ -1163,10 +1166,17 @@ def test_prefetch_passes_targets_and_tools_to_script(tmp_path: Path) -> None:
     cmd = run.call_args[0][0]
     assert cmd[-3:] == ["esp32,esp32c3", "required", "cmake"]
     assert cmd[1].endswith("get_tool_downloads.py")
-    # the script inherits the caller's env plus the framework tools PYTHONPATH
+    # the script inherits the caller's env plus an explicit PYTHONPATH:
+    # sibling scripts, the esphome package root, the framework's idf_tools
     env = run.call_args[1]["env"]
     assert env["IDF_TOOLS_PATH"] == "/x"
-    assert env["PYTHONPATH"] == str(tmp_path / "tools")
+    assert env["PYTHONPATH"] == os.pathsep.join(
+        (
+            str(_ESPIDF_SCRIPTS_DIR),
+            str(_ESPIDF_SCRIPTS_DIR.parents[1]),
+            str(tmp_path / "tools"),
+        )
+    )
 
 
 def test_framework_install_prefetches_before_installer(
@@ -2202,3 +2212,28 @@ def test_install_tool_archives_inprocess_dedupes_and_skips(
     assert (tools / "ninja" / "1.12.1" / ".installed").is_file()
     assert not (tools / "installed-tool").exists()
     assert not (tools / "broken-tool").exists()
+
+
+def test_install_tool_archives_surviving_torn_dir_escalates(
+    tmp_path: Path,
+    monkeypatch: pytest.MonkeyPatch,
+    capsys: pytest.CaptureFixture[str],
+) -> None:
+    """A torn dir that survives cleanup could fool the installer; the exit
+    is nonzero even though the other tool succeeded."""
+    import esphome.helpers
+
+    _make_dist(tmp_path, "cmake.tar.gz", "ninja-v1.zip")
+    monkeypatch.setenv("TEST_FAIL_INSTALL", "ninja")
+    monkeypatch.setattr(
+        esphome.helpers, "rmtree", MagicMock(side_effect=OSError("busy"))
+    )
+    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
+    err = capsys.readouterr().err
+    assert "could not remove" in err
+    assert "1 of 2 pre-extractions failed" in err
+    assert (tmp_path / "tp" / "tools" / "cmake" / "3.30.2" / ".installed").is_file()