[git] Fix device adoption failing on first attempt: lock the clone cache against concurrent resolutions (#17923)

esphome/components/packages/__init__.py

@@ -1,6 +1,7 @@
 from collections import UserDict
 from collections.abc import Callable
 from functools import reduce
+import logging
 from pathlib import Path
 from typing import Any
 
@@ -35,6 +36,8 @@ from esphome.const import (
 )
 from esphome.core import EsphomeError
 
+_LOGGER = logging.getLogger(__name__)
+
 DOMAIN = CONF_PACKAGES
 # Guard against infinite include chains (e.g. A includes B includes A).
 MAX_INCLUDE_DEPTH = 20
@@ -267,8 +270,23 @@ def _process_remote_package(config: dict[str, Any]) -> dict[str, Any]:
         # If loading fails, the cached checkout may be stale — revert and retry once.
         try:
             return {CONF_PACKAGES: get_packages(files)}
-        except cv.Invalid:
-            revert()
+        except cv.Invalid as err:
+            if not revert():
+                # The pre-update content is out of reach (lock timeout, the
+                # checkout moved, or the reset failed; see the log), so a
+                # retry could not see it.
+                raise cv.Invalid(
+                    f"Failed to load packages and could not revert the cached "
+                    f"checkout to retry. {err}",
+                    path=err.path,
+                ) from err
+            # If the retry succeeds this is the only trace that the
+            # refreshed upstream content was broken.
+            _LOGGER.warning(
+                "Loading packages failed (%s), reverted the cached checkout "
+                "and retrying",
+                err,
+            )
         try:
             return {CONF_PACKAGES: get_packages(files)}
         except cv.Invalid as err:

requirements.txt

@@ -27,7 +27,7 @@ smpclient==7.2.0
 requests==2.34.2
 py7zr==1.1.3
 platformdirs==4.11.0  # native esp-idf toolchain global cache dir
-filelock==3.32.0  # lock guarding the PlatformIO python-version cache heal
+filelock==3.32.0  # inter-process locks (PlatformIO cache heal, git clone cache); >=3.32 for FileLock(fallback_to_soft=...), older versions silently drop the kwarg
 
 # esp-idf >= 5.0 requires this
 pyparsing >= 3.3.2

tests/component_tests/packages/test_packages.py

@@ -1264,6 +1264,75 @@ def test_remote_packages_no_revert(
     ]
 
 
+@patch("esphome.yaml_util.load_yaml")
+@patch("pathlib.Path.is_file")
+@patch("esphome.git.clone_or_update")
+def test_remote_packages_skipped_revert_does_not_retry(
+    mock_clone_or_update, mock_is_file, mock_load_yaml
+) -> None:
+    """When revert() reports the rollback was skipped, the load is not
+    retried (the checkout is unchanged) and the error says so."""
+    mock_revert = MagicMock(return_value=False)
+    mock_clone_or_update.return_value = (Path("/tmp/noexists"), mock_revert)
+    mock_is_file.return_value = True
+    mock_load_yaml.side_effect = cv.Invalid("bad yaml")
+
+    config = {
+        CONF_PACKAGES: {
+            "pkg": {
+                CONF_URL: "https://github.com/esphome/repo",
+                CONF_REF: "main",
+                CONF_FILES: [{CONF_PATH: "file.yaml"}],
+                CONF_REFRESH: "1d",
+            }
+        }
+    }
+    with pytest.raises(cv.Invalid, match="could not revert the cached checkout"):
+        packages_pass(config)
+
+    assert mock_revert.call_count == 1
+    assert mock_load_yaml.call_count == 1
+
+
+@patch("esphome.yaml_util.load_yaml")
+@patch("pathlib.Path.is_file")
+@patch("esphome.git.clone_or_update")
+def test_remote_packages_successful_revert_retries(
+    mock_clone_or_update, mock_is_file, mock_load_yaml, caplog: pytest.LogCaptureFixture
+) -> None:
+    """A successful revert retries the load against the reverted checkout and
+    logs the original error, the only trace that upstream was broken."""
+    mock_revert = MagicMock(return_value=True)
+    mock_clone_or_update.return_value = (Path("/tmp/noexists"), mock_revert)
+    mock_is_file.return_value = True
+    mock_load_yaml.side_effect = [
+        cv.Invalid("bad yaml"),
+        OrderedDict(
+            {CONF_SENSOR: [{CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, CONF_NAME: "test"}]}
+        ),
+    ]
+
+    config = {
+        CONF_PACKAGES: {
+            "pkg": {
+                CONF_URL: "https://github.com/esphome/repo",
+                CONF_REF: "main",
+                CONF_FILES: [{CONF_PATH: "file.yaml"}],
+                CONF_REFRESH: "1d",
+            }
+        }
+    }
+    with caplog.at_level(logging.WARNING):
+        actual = packages_pass(config)
+
+    assert actual[CONF_SENSOR] == [
+        {CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, CONF_NAME: "test"}
+    ]
+    assert mock_revert.call_count == 1
+    assert mock_load_yaml.call_count == 2
+    assert any("reverted the cached checkout" in r.getMessage() for r in caplog.records)
+
+
 def test_raw_config_contains_merged_esphome_from_package(tmp_path) -> None:
     """Test that CORE.raw_config contains esphome section from merged package.