diff --git a/esphome/git.py b/esphome/git.py index c4a612753b..0c1ad56367 100644 --- a/esphome/git.py +++ b/esphome/git.py @@ -10,14 +10,22 @@ import time import urllib.parse import esphome.config_validation as cv -from esphome.core import CORE, TimePeriodSeconds -from esphome.helpers import rmtree +from esphome.core import CORE, EsphomeError, TimePeriodSeconds +from esphome.helpers import rmtree, write_file _LOGGER = logging.getLogger(__name__) # Special value to indicate never refresh NEVER_REFRESH = TimePeriodSeconds(seconds=-1) +# Written inside .git only after every clone step (clone, ref fetch, reset, +# submodule init) has completed. A directory without it is an interrupted +# clone (e.g. the process was killed mid-clone) and must be re-cloned; without +# this check such a directory would be trusted forever when the caller uses +# NEVER_REFRESH. Lives in .git so stash/reset/checkout can never touch it and +# it does not pollute the worktree. +_CLONE_COMPLETE_MARKER = "esphome_clone_complete" + class GitException(cv.Invalid): """Base exception for git-related errors.""" @@ -95,6 +103,26 @@ def _compute_destination_path(key: str, domain: str) -> Path: return base_dir / h.hexdigest()[:8] +def _clone_complete_marker_path(repo_dir: Path) -> Path: + return repo_dir / ".git" / _CLONE_COMPLETE_MARKER + + +def _remove_repo_dir(repo_dir: Path) -> None: + """Remove a repo directory, deleting the completion marker first. + + Marker-first ordering guarantees an interrupted removal can never leave a + marker behind next to a partially deleted worktree. The unlink is best + effort: if it fails (e.g. a file lock on Windows), rmtree below still + gets the chance to remove the directory, marker included. + """ + try: + _clone_complete_marker_path(repo_dir).unlink(missing_ok=True) + except OSError as err: + _LOGGER.debug("Could not delete clone completion marker first: %s", err) + if repo_dir.is_dir(): + rmtree(repo_dir) + + def resolve_symlink_stub(repo_dir: Path, file_path: Path) -> Path | None: """Return the symlink target if ``file_path`` is a Windows-checked-out symlink stub. @@ -201,9 +229,19 @@ def clone_or_update( ) repo_dir = _compute_destination_path(key, domain) + hash_dir_name = repo_dir.name if subpath: repo_dir = repo_dir / subpath + if repo_dir.is_dir() and not _clone_complete_marker_path(repo_dir).is_file(): + # The last clone never finished (killed process, container stop) or + # predates the marker; either way it cannot be trusted, especially + # with NEVER_REFRESH where it would otherwise be reused forever. + _LOGGER.warning( + "Removing incomplete clone of %s at %s, will re-clone", key, repo_dir + ) + _remove_repo_dir(repo_dir) + if not repo_dir.is_dir(): _LOGGER.info("Cloning %s", key) _LOGGER.debug("Location: %s", repo_dir) @@ -233,14 +271,28 @@ def clone_or_update( + submodules, git_dir=repo_dir, ) + except GitException: # Remove incomplete clone to prevent stale state. Without this, # a failed ref fetch leaves a clone on the default branch, and # subsequent calls skip the update due to the refresh window. - if repo_dir.is_dir(): - rmtree(repo_dir) + _remove_repo_dir(repo_dir) raise + # Every git step succeeded; the key and hash dir name are recorded + # purely to make cache debugging easier. The marker is only a + # validity signal, so a failed write must not fail an otherwise + # complete clone: the only cost is a re-clone on the next run. + try: + write_file( + _clone_complete_marker_path(repo_dir), + f"key={key}\nhash={hash_dir_name}\n", + ) + except EsphomeError as err: + _LOGGER.warning( + "Could not write clone completion marker for %s: %s", key, err + ) + else: if refresh == NEVER_REFRESH or CORE.skip_external_update: _LOGGER.debug("Skipping update for %s (refresh disabled)", key) @@ -250,7 +302,13 @@ def clone_or_update( # On first clone, FETCH_HEAD does not exist if not file_timestamp.exists(): file_timestamp = Path(repo_dir / ".git" / "HEAD") - age_seconds = time.time() - file_timestamp.stat().st_mtime + try: + age_seconds = time.time() - file_timestamp.stat().st_mtime + except OSError: + # A .git with neither FETCH_HEAD nor HEAD is corrupt (e.g. a + # partially deleted clone). Force the update path so the + # broken-repository recovery below removes and re-clones it. + age_seconds = float("inf") if refresh is None or age_seconds > refresh.total_seconds: # Try to update the repository, recovering from broken state if needed old_sha: str | None = None @@ -303,7 +361,7 @@ def clone_or_update( err, ) _LOGGER.info("Removing broken repository at %s", repo_dir) - rmtree(repo_dir) + _remove_repo_dir(repo_dir) _LOGGER.info("Successfully removed broken repository, re-cloning...") # Recursively call clone_or_update to re-clone @@ -316,6 +374,7 @@ def clone_or_update( username=username, password=password, submodules=submodules, + subpath=subpath, _recover_broken=False, ) _LOGGER.info("Repository %s successfully recovered", key) diff --git a/tests/unit_tests/test_git.py b/tests/unit_tests/test_git.py index 62d2344069..c9e0339ad7 100644 --- a/tests/unit_tests/test_git.py +++ b/tests/unit_tests/test_git.py @@ -1,5 +1,6 @@ """Tests for git.py module.""" +from collections.abc import Callable import os from pathlib import Path import time @@ -9,7 +10,7 @@ from unittest.mock import Mock, patch import pytest from esphome import git -from esphome.core import CORE, TimePeriodSeconds +from esphome.core import CORE, EsphomeError, TimePeriodSeconds from esphome.git import GitCommandError @@ -19,6 +20,15 @@ def _compute_repo_dir(url: str, ref: str | None, domain: str) -> Path: return git._compute_destination_path(key, domain) +# The tests must probe the exact location the implementation uses +_marker_path = git._clone_complete_marker_path + + +def _mark_clone_complete(repo_dir: Path) -> None: + """Write the completion marker so a hand-made repo dir is treated as valid.""" + _marker_path(repo_dir).write_text("test") + + def _setup_old_repo(repo_dir: Path, days_old: int = 2) -> None: """Helper to set up a git repo directory structure with an old timestamp. @@ -30,6 +40,7 @@ def _setup_old_repo(repo_dir: Path, days_old: int = 2) -> None: repo_dir.mkdir(parents=True) git_dir = repo_dir / ".git" git_dir.mkdir() + _mark_clone_complete(repo_dir) # Create FETCH_HEAD file with old timestamp fetch_head = git_dir / "FETCH_HEAD" @@ -54,6 +65,25 @@ def _get_git_command_type(cmd: list[str]) -> str | None: return None +def _simulate_cloned_repo(repo_dir: Path) -> None: + """Create the directory structure a successful git clone would leave.""" + repo_dir.mkdir(parents=True, exist_ok=True) + (repo_dir / ".git").mkdir(exist_ok=True) + + +def _make_clone_side_effect(repo_dir: Path) -> Callable[..., str]: + """Return a run_git_command side effect whose clone creates the repo dir.""" + + def git_command_side_effect( + cmd: list[str], cwd: str | None = None, **kwargs: Any + ) -> str: + if _get_git_command_type(cmd) == "clone": + _simulate_cloned_repo(repo_dir) + return "" + + return git_command_side_effect + + def test_run_git_command_success(tmp_path: Path) -> None: """Test that run_git_command returns output on success.""" # Create a simple git repo to test with @@ -217,6 +247,7 @@ def test_clone_or_update_with_never_refresh( repo_dir.mkdir(parents=True) git_dir = repo_dir / ".git" git_dir.mkdir() + _mark_clone_complete(repo_dir) # Create FETCH_HEAD file with current timestamp fetch_head = git_dir / "FETCH_HEAD" @@ -250,6 +281,7 @@ def test_clone_or_update_skips_when_core_skip_external_update( repo_dir.mkdir(parents=True) git_dir = repo_dir / ".git" git_dir.mkdir() + _mark_clone_complete(repo_dir) (git_dir / "FETCH_HEAD").write_text("test") CORE.skip_external_update = True @@ -281,6 +313,7 @@ def test_clone_or_update_with_refresh_updates_old_repo( repo_dir.mkdir(parents=True) git_dir = repo_dir / ".git" git_dir.mkdir() + _mark_clone_complete(repo_dir) # Create FETCH_HEAD file with old timestamp (2 days ago) fetch_head = git_dir / "FETCH_HEAD" @@ -329,6 +362,7 @@ def test_clone_or_update_with_refresh_skips_fresh_repo( repo_dir.mkdir(parents=True) git_dir = repo_dir / ".git" git_dir.mkdir() + _mark_clone_complete(repo_dir) # Create FETCH_HEAD file with recent timestamp (1 hour ago) fetch_head = git_dir / "FETCH_HEAD" @@ -371,6 +405,8 @@ def test_clone_or_update_clones_missing_repo( # repo_dir should NOT exist assert not repo_dir.exists() + mock_run_git_command.side_effect = _make_clone_side_effect(repo_dir) + # Test with NEVER_REFRESH - should still clone since repo doesn't exist result_dir, revert = git.clone_or_update( url=url, @@ -405,6 +441,7 @@ def test_clone_or_update_with_none_refresh_always_updates( repo_dir.mkdir(parents=True) git_dir = repo_dir / ".git" git_dir.mkdir() + _mark_clone_complete(repo_dir) # Create FETCH_HEAD file with very recent timestamp (1 second ago) fetch_head = git_dir / "FETCH_HEAD" @@ -486,6 +523,9 @@ def test_clone_or_update_recovers_from_git_failures( # Default successful responses if cmd_type == "rev-parse": return "abc123" + if cmd_type == "clone": + # Simulate the recovery re-clone creating the repo directory + _simulate_cloned_repo(repo_dir) return "" mock_run_git_command.side_effect = git_command_side_effect @@ -813,6 +853,273 @@ def test_clone_or_update_stale_clone_is_retried_after_cleanup( assert call_count["fetch"] == 2 +def test_clone_or_update_recloned_when_marker_missing_with_never_refresh( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """A repo dir without the completion marker is an interrupted clone. + + It must be removed and re-cloned even with NEVER_REFRESH, which would + otherwise trust the broken directory forever. + """ + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + ref = "1.8.4" + domain = "test" + repo_dir = _compute_repo_dir(url, ref, domain) + + # Simulate an interrupted clone: directory exists, no marker + repo_dir.mkdir(parents=True) + (repo_dir / ".git").mkdir() + + mock_run_git_command.side_effect = _make_clone_side_effect(repo_dir) + + result_dir, _ = git.clone_or_update( + url=url, ref=ref, refresh=git.NEVER_REFRESH, domain=domain + ) + + clone_calls = [c for c in mock_run_git_command.call_args_list if "clone" in c[0][0]] + assert len(clone_calls) == 1 + assert result_dir == repo_dir + # The fresh clone completed, so the marker must now be present + assert _marker_path(repo_dir).is_file() + + +def test_clone_or_update_recloned_when_marker_missing_with_skip_external_update( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """skip_external_update must not preserve an interrupted clone.""" + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + domain = "test" + repo_dir = _compute_repo_dir(url, None, domain) + + repo_dir.mkdir(parents=True) + (repo_dir / ".git").mkdir() + + mock_run_git_command.side_effect = _make_clone_side_effect(repo_dir) + + CORE.skip_external_update = True + result_dir, _ = git.clone_or_update( + url=url, ref=None, refresh=TimePeriodSeconds(days=1), domain=domain + ) + + clone_calls = [c for c in mock_run_git_command.call_args_list if "clone" in c[0][0]] + assert len(clone_calls) == 1 + assert result_dir == repo_dir + assert _marker_path(repo_dir).is_file() + + +def test_fresh_clone_writes_completion_marker_with_debug_info( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """The marker is written after a fresh clone and records key and hash dir.""" + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + ref = "main" + domain = "test" + repo_dir = _compute_repo_dir(url, ref, domain) + + mock_run_git_command.side_effect = _make_clone_side_effect(repo_dir) + + git.clone_or_update(url=url, ref=ref, refresh=git.NEVER_REFRESH, domain=domain) + + marker = _marker_path(repo_dir) + assert marker.is_file() + content = marker.read_text() + assert f"{url}@{ref}" in content + assert repo_dir.name in content + + +def test_marker_is_deleted_before_rmtree( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """The marker must be gone even if rmtree fails partway. + + Simulated by an rmtree that does nothing: the directory survives but the + marker must already have been deleted, so the next run still re-clones + instead of trusting a partially deleted worktree. + """ + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + ref = "main" + domain = "test" + repo_dir = _compute_repo_dir(url, ref, domain) + + _setup_old_repo(repo_dir) + assert _marker_path(repo_dir).is_file() + + def git_command_side_effect( + cmd: list[str], cwd: str | None = None, **kwargs: Any + ) -> str: + if _get_git_command_type(cmd) == "stash": + raise GitCommandError("fatal: unable to write new index file") + return "abc123" + + mock_run_git_command.side_effect = git_command_side_effect + + with ( + patch("esphome.git.rmtree"), + pytest.raises(GitCommandError), + ): + git.clone_or_update( + url=url, ref=ref, refresh=TimePeriodSeconds(days=1), domain=domain + ) + + # rmtree never deleted anything, yet the marker is gone + assert repo_dir.is_dir() + assert not _marker_path(repo_dir).is_file() + + +def test_failed_marker_write_does_not_fail_the_clone( + tmp_path: Path, + mock_run_git_command: Mock, + caplog: pytest.LogCaptureFixture, +) -> None: + """A marker write failure must not fail an otherwise complete clone. + + The clone is valid; the missing marker only costs a re-clone on the next + run, so the error is logged as a warning instead of propagating. + """ + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + domain = "test" + repo_dir = _compute_repo_dir(url, None, domain) + + mock_run_git_command.side_effect = _make_clone_side_effect(repo_dir) + + with patch( + "esphome.git.write_file", side_effect=EsphomeError("Could not write file") + ): + result_dir, _ = git.clone_or_update( + url=url, ref=None, refresh=git.NEVER_REFRESH, domain=domain + ) + + assert result_dir == repo_dir + assert not _marker_path(repo_dir).is_file() + assert "Could not write clone completion marker" in caplog.text + + +def test_corrupt_git_dir_without_head_recovers( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """A .git with neither FETCH_HEAD nor HEAD must recover, not crash. + + The age check stats FETCH_HEAD falling back to HEAD; if both are gone + (partially deleted clone) the stat raised an unhandled FileNotFoundError + before the broken-repository recovery could run. + """ + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + domain = "test" + repo_dir = _compute_repo_dir(url, None, domain) + + # Marker present but .git gutted: no FETCH_HEAD, no HEAD + repo_dir.mkdir(parents=True) + (repo_dir / ".git").mkdir() + _mark_clone_complete(repo_dir) + + def git_command_side_effect( + cmd: list[str], cwd: str | None = None, **kwargs: Any + ) -> str: + cmd_type = _get_git_command_type(cmd) + if cmd_type == "rev-parse": + raise GitCommandError("ambiguous argument 'HEAD': unknown revision") + if cmd_type == "clone": + _simulate_cloned_repo(repo_dir) + return "" + + mock_run_git_command.side_effect = git_command_side_effect + + result_dir, _ = git.clone_or_update( + url=url, ref=None, refresh=TimePeriodSeconds(days=1), domain=domain + ) + + assert result_dir == repo_dir + clone_calls = [c for c in mock_run_git_command.call_args_list if "clone" in c[0][0]] + assert len(clone_calls) == 1 + assert _marker_path(repo_dir).is_file() + + +def test_remove_repo_dir_tolerates_marker_unlink_failure(tmp_path: Path) -> None: + """A locked marker file must not abort the directory removal.""" + repo_dir = tmp_path / "repo" + repo_dir.mkdir() + (repo_dir / ".git").mkdir() + _mark_clone_complete(repo_dir) + + with patch.object(Path, "unlink", side_effect=PermissionError("locked")): + git._remove_repo_dir(repo_dir) + + # rmtree still removed the directory, marker included + assert not repo_dir.exists() + + +def test_clone_or_update_recovery_preserves_subpath( + tmp_path: Path, mock_run_git_command: Mock +) -> None: + """Recovery must re-clone into the same subpath-ed directory. + + Without passing subpath through, the recursive recovery call would + recompute the destination without the subpath and clone (and write the + completion marker) at the wrong location. + """ + CORE.config_path = tmp_path / "test.yaml" + + url = "https://github.com/test/repo" + ref = "main" + domain = "test" + subpath = Path("mylib") + repo_dir = _compute_repo_dir(url, ref, domain) / subpath + + _setup_old_repo(repo_dir) + + call_counts: dict[str, int] = {} + + def git_command_side_effect( + cmd: list[str], cwd: str | None = None, **kwargs: Any + ) -> str: + cmd_type = _get_git_command_type(cmd) + if cmd_type: + call_counts[cmd_type] = call_counts.get(cmd_type, 0) + 1 + # First rev-parse fails (broken repo) to trigger recovery + if cmd_type == "rev-parse" and call_counts[cmd_type] == 1: + raise GitCommandError( + "ambiguous argument 'HEAD': unknown revision or path not in the working tree." + ) + if cmd_type == "clone": + # Create whatever directory the clone was asked to target + target = Path(cmd[-1]) + target.mkdir(parents=True, exist_ok=True) + (target / ".git").mkdir(exist_ok=True) + if cmd_type == "rev-parse": + return "abc123" + return "" + + mock_run_git_command.side_effect = git_command_side_effect + + result_dir, _ = git.clone_or_update( + url=url, + ref=ref, + refresh=TimePeriodSeconds(days=1), + domain=domain, + subpath=subpath, + ) + + # The recovery re-clone must target the subpath-ed directory and the + # completion marker must land there too + clone_calls = [c for c in mock_run_git_command.call_args_list if "clone" in c[0][0]] + assert len(clone_calls) == 1 + assert clone_calls[0][0][0][-1] == str(repo_dir) + assert result_dir == repo_dir + assert _marker_path(repo_dir).is_file() + + def test_clone_with_ref_uses_shallow_fetch( tmp_path: Path, mock_run_git_command: Mock ) -> None: