From fe1491fd26f915f23e0258134696725cd6d12d0b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 30 Sep 2026 13:43:50 +0200 Subject: [PATCH] [espidf] Stage the sync and promote files into the mirror with renames The manager writes its files in place, so syncing into the live mirror could expose a truncated index to a process configuring at the same time; renames are atomic, and the staged index is merged with the mirror's existing versions before promotion. --- esphome/espidf/component_mirror.py | 59 ++++++++++++++++--- .../test_espidf_component_mirror.py | 58 +++++++++++++++++- 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/esphome/espidf/component_mirror.py b/esphome/espidf/component_mirror.py index d5b9260f9e7..e510a9dc16d 100644 --- a/esphome/espidf/component_mirror.py +++ b/esphome/espidf/component_mirror.py @@ -13,6 +13,7 @@ import json import logging import os from pathlib import Path +import shutil import subprocess from typing import TYPE_CHECKING, NamedTuple @@ -35,6 +36,7 @@ _ENV_CHECK_NEW_VERSION = "IDF_COMPONENT_CHECK_NEW_VERSION" _DEFAULT_REGISTRY_URL = "https://components.espressif.com" _SYNC_LOCK_NAME = ".sync.lock" +_STAGING_DIR_NAME = ".staging" _SYNC_TIMEOUT_S = 120 @@ -182,6 +184,40 @@ def missing_deps(mirror: Path, deps: list[ServiceDep]) -> list[ServiceDep]: return [dep for dep in deps if not _mirror_has(mirror, dep)] +def _merge_component_index(src: Path, dst: Path) -> None: + """Fold the mirror's existing versions of a component into the staged index. + + The staged index lists only the versions this sync fetched; replacing + the mirror's file outright would drop the versions it already had. + """ + try: + existing = json.loads(dst.read_text(encoding="utf-8"))["versions"] + staged = json.loads(src.read_text(encoding="utf-8")) + known = {entry.get("version") for entry in staged["versions"]} + staged["versions"] += [ + entry for entry in existing if entry.get("version") not in known + ] + src.write_text(json.dumps(staged), encoding="utf-8") + except (OSError, ValueError, TypeError, KeyError, AttributeError): + # No usable existing index; the staged one stands alone. + pass + + +def _promote(staging: Path, mirror: Path) -> None: + """Move the synced files into the mirror, one atomic rename each.""" + for src in sorted(path for path in staging.rglob("*") if path.is_file()): + rel = src.relative_to(staging) + dst = mirror / rel + dst.parent.mkdir(parents=True, exist_ok=True) + if ( + rel.parts[0] == "components" + and len(rel.parts) == 3 + and rel.suffix == ".json" + ): + _merge_component_index(src, dst) + Path(src).replace(dst) + + def sync_component_mirror( lock_path: Path, manifest_path: Path, @@ -206,10 +242,14 @@ def sync_component_mirror( except (OSError, EsphomeError) as err: _LOGGER.warning("Could not mirror IDF components: %s", err) return False + # Sync into a staging directory: the manager writes files in place, so + # a configure in another process could read a truncated file from the + # live mirror. Promoting with renames keeps every read consistent. + staging = mirror / _STAGING_DIR_NAME cmd = [python, "-m", "idf_component_manager", "registry", "sync"] for dep in to_sync: cmd += ["--component", f"{dep.namespace}/{dep.name}=={dep.version}"] - cmd.append(str(mirror)) + cmd.append(str(staging)) # Lazy import, as in git.py: keeps filelock off the CLI startup path. from filelock import FileLock @@ -223,6 +263,7 @@ def sync_component_mirror( return True _LOGGER.info("Mirroring %d IDF component(s) for offline builds...", len(to_sync)) try: + shutil.rmtree(staging, ignore_errors=True) result = subprocess.run( cmd, env=env, @@ -231,16 +272,20 @@ def sync_component_mirror( timeout=_SYNC_TIMEOUT_S, check=False, ) + if result.returncode != 0: + tail = "\n".join((result.stderr or result.stdout).strip().splitlines()[-5:]) + _LOGGER.warning( + "Could not mirror IDF components (exit %d):\n%s", + result.returncode, + tail, + ) + return False + _promote(staging, mirror) except (OSError, subprocess.SubprocessError) as err: _LOGGER.warning("Could not mirror IDF components: %s", err) return False finally: + shutil.rmtree(staging, ignore_errors=True) lock.release() - if result.returncode != 0: - tail = "\n".join((result.stderr or result.stdout).strip().splitlines()[-5:]) - _LOGGER.warning( - "Could not mirror IDF components (exit %d):\n%s", result.returncode, tail - ) - return False _LOGGER.info("Mirrored %d IDF component(s) for offline builds", len(to_sync)) return True diff --git a/tests/unit_tests/test_espidf_component_mirror.py b/tests/unit_tests/test_espidf_component_mirror.py index 652df1683eb..b9cec733998 100644 --- a/tests/unit_tests/test_espidf_component_mirror.py +++ b/tests/unit_tests/test_espidf_component_mirror.py @@ -313,7 +313,7 @@ def test_sync_runs_the_manager_for_missing_deps(tmp_path: Path) -> None: "bblanchon/arduinojson==7.4.3", "--component", "espressif/mdns==1.12.0", - str(mirror), + str(mirror / ".staging"), ] assert mock_run.call_args.kwargs["env"] == {"PATH": "/penv"} _assert_sync_lock_released() @@ -389,6 +389,62 @@ def test_sync_skips_when_another_process_holds_the_lock(tmp_path: Path) -> None: mock_run.assert_not_called() +def _fake_registry_sync(returncode: int = 0): + """A subprocess.run stand-in that lays files out like `registry sync`.""" + + def run(cmd, **kwargs) -> subprocess.CompletedProcess: + staging = Path(cmd[-1]) + _add_to_mirror(staging, component_mirror.ServiceDep("ns", "cmp", "2.0.0")) + return subprocess.CompletedProcess(cmd, returncode, "", "sync failed") + + return run + + +def _sync_custom_lock(tmp_path: Path, returncode: int = 0) -> bool: + lock = _write_lock( + tmp_path, + "dependencies:\n" + " ns/cmp:\n" + " source:\n" + " type: service\n" + " version: 2.0.0\n", + ) + with patch.object( + component_mirror.subprocess, "run", side_effect=_fake_registry_sync(returncode) + ): + return component_mirror.sync_component_mirror( + lock, + tmp_path / "src" / "idf_component.yml", + lambda: "/penv/python", + dict, + ) + + +def test_sync_promotes_staged_files_and_merges_the_index(tmp_path: Path) -> None: + """New files land through renames and existing versions survive the merge, + so a concurrent configure never reads a truncated index.""" + mirror = component_mirror.get_mirror_path() + _add_to_mirror(mirror, component_mirror.ServiceDep("ns", "cmp", "1.0.0")) + + assert _sync_custom_lock(tmp_path) + + doc = json.loads((mirror / "components" / "ns" / "cmp.json").read_text()) + assert {entry["version"] for entry in doc["versions"]} == {"1.0.0", "2.0.0"} + assert (mirror / "components/ns/cmp/2.0.0/ns__cmp-v2.0.0.zip").is_file() + assert (mirror / "components/ns/cmp/1.0.0/ns__cmp-v1.0.0.zip").is_file() + assert not (mirror / ".staging").exists() + + +def test_sync_failure_leaves_no_staging_behind(tmp_path: Path) -> None: + """A failed sync must not leave partial downloads for the next coverage + check to mistake for a mirror.""" + assert not _sync_custom_lock(tmp_path, returncode=1) + + mirror = component_mirror.get_mirror_path() + assert not (mirror / ".staging").exists() + assert not (mirror / "components" / "ns").exists() + + def test_sync_ignores_a_leftover_lock_file(tmp_path: Path) -> None: """A lock file from a dead process does not block: the OS lock is gone.""" _write_lock(tmp_path)