From 6cebf522491eea6a45358a8fb1ded774f698407b Mon Sep 17 00:00:00 2001 From: Mike Degatano Date: Wed, 2 Jul 2025 14:34:18 -0400 Subject: [PATCH] Store reset only deletes git cache after clone was successful (#5984) * Store reset only deletes git cache after clone was successful * Add test and fix fallback error handling * Fix when lock is grabbed --- .../resolution/fixups/store_execute_reset.py | 12 +- supervisor/store/git.py | 103 ++++++++---- .../fixup/test_store_execute_reset.py | 146 +++++++++++++++--- 3 files changed, 202 insertions(+), 59 deletions(-) diff --git a/supervisor/resolution/fixups/store_execute_reset.py b/supervisor/resolution/fixups/store_execute_reset.py index 12597cc31..c65bee95e 100644 --- a/supervisor/resolution/fixups/store_execute_reset.py +++ b/supervisor/resolution/fixups/store_execute_reset.py @@ -1,6 +1,5 @@ """Helpers to check and fix issues with free space.""" -from functools import partial import logging from ...coresys import CoreSys @@ -12,7 +11,6 @@ from ...exceptions import ( ) from ...jobs.const import JobCondition from ...jobs.decorator import Job -from ...utils import remove_folder from ..const import ContextType, IssueType, SuggestionType from .base import FixupBase @@ -45,13 +43,11 @@ class FixupStoreExecuteReset(FixupBase): return # Local add-ons are not a git repo, can't remove and re-pull - if repository.git: - await self.sys_run_in_executor( - partial(remove_folder, folder=repository.git.path, content_only=True) - ) - - # Load data again try: + if repository.git: + await repository.git.reset() + + # Load data again await repository.load() except StoreError: raise ResolutionFixupError() from None diff --git a/supervisor/store/git.py b/supervisor/store/git.py index 8ed980fdc..9e25da927 100644 --- a/supervisor/store/git.py +++ b/supervisor/store/git.py @@ -2,9 +2,11 @@ from abc import ABC, abstractmethod import asyncio +import errno import functools as ft import logging from pathlib import Path +from tempfile import TemporaryDirectory import git @@ -12,7 +14,7 @@ from ..const import ATTR_BRANCH, ATTR_URL from ..coresys import CoreSys, CoreSysAttributes from ..exceptions import StoreGitCloneError, StoreGitError, StoreJobError from ..jobs.decorator import Job, JobCondition -from ..resolution.const import ContextType, IssueType, SuggestionType +from ..resolution.const import ContextType, IssueType, SuggestionType, UnhealthyReason from ..utils import remove_folder from .utils import get_hash_from_repository from .validate import RE_REPOSITORY, BuiltinRepository @@ -88,38 +90,77 @@ class GitRepo(CoreSysAttributes, ABC): async def clone(self) -> None: """Clone git add-on repository.""" async with self.lock: - git_args = { - attribute: value - for attribute, value in ( - ("recursive", True), - ("branch", self.branch), - ("depth", 1), - ("shallow-submodules", True), - ) - if value is not None - } + await self._clone() - try: - _LOGGER.info( - "Cloning add-on %s repository from %s", self.path, self.url - ) - self.repo = await self.sys_run_in_executor( - ft.partial( - git.Repo.clone_from, - self.url, - str(self.path), - **git_args, # type: ignore - ) - ) + @Job( + name="git_repo_reset", + conditions=[JobCondition.FREE_SPACE, JobCondition.INTERNET_SYSTEM], + on_condition=StoreJobError, + ) + async def reset(self) -> None: + """Reset repository to fix issue with local copy.""" + # Clone into temporary folder + temp_dir = await self.sys_run_in_executor( + TemporaryDirectory, dir=self.sys_config.path_tmp + ) + temp_path = Path(temp_dir.name) + try: + await self._clone(temp_path) - except ( - git.InvalidGitRepositoryError, - git.NoSuchPathError, - git.CommandError, - UnicodeDecodeError, - ) as err: - _LOGGER.error("Can't clone %s repository: %s.", self.url, err) - raise StoreGitCloneError() from err + # Remove corrupted repo and move temp clone to its place + def move_clone(): + remove_folder(folder=self.path) + temp_path.rename(self.path) + + async with self.lock: + try: + await self.sys_run_in_executor(move_clone) + except OSError as err: + if err.errno == errno.EBADMSG: + self.sys_resolution.add_unhealthy_reason( + UnhealthyReason.OSERROR_BAD_MESSAGE + ) + raise StoreGitCloneError( + f"Can't move clone due to: {err!s}", _LOGGER.error + ) from err + finally: + # Clean up temporary directory in case of error + # If the folder was moved this will do nothing + await self.sys_run_in_executor(temp_dir.cleanup) + + async def _clone(self, path: Path | None = None) -> None: + """Clone git add-on repository to location.""" + path = path or self.path + git_args = { + attribute: value + for attribute, value in ( + ("recursive", True), + ("branch", self.branch), + ("depth", 1), + ("shallow-submodules", True), + ) + if value is not None + } + + try: + _LOGGER.info("Cloning add-on %s repository from %s", path, self.url) + self.repo = await self.sys_run_in_executor( + ft.partial( + git.Repo.clone_from, + self.url, + str(path), + **git_args, # type: ignore + ) + ) + + except ( + git.InvalidGitRepositoryError, + git.NoSuchPathError, + git.CommandError, + UnicodeDecodeError, + ) as err: + _LOGGER.error("Can't clone %s repository: %s.", self.url, err) + raise StoreGitCloneError() from err @Job( name="git_repo_pull", diff --git a/tests/resolution/fixup/test_store_execute_reset.py b/tests/resolution/fixup/test_store_execute_reset.py index 2856c6a2c..361fa9d04 100644 --- a/tests/resolution/fixup/test_store_execute_reset.py +++ b/tests/resolution/fixup/test_store_execute_reset.py @@ -1,43 +1,149 @@ """Test evaluation base.""" # pylint: disable=import-error,protected-access +import errno from os import listdir from pathlib import Path -from unittest.mock import AsyncMock, patch +from unittest.mock import PropertyMock, patch +import pytest + +from supervisor.config import CoreConfig from supervisor.coresys import CoreSys -from supervisor.resolution.const import ContextType, IssueType, SuggestionType +from supervisor.exceptions import StoreGitCloneError +from supervisor.resolution.const import ( + ContextType, + IssueType, + SuggestionType, + UnhealthyReason, +) from supervisor.resolution.data import Issue, Suggestion from supervisor.resolution.fixups.store_execute_reset import FixupStoreExecuteReset +from supervisor.store.git import GitRepo +from supervisor.store.repository import Repository -async def test_fixup(coresys: CoreSys, supervisor_internet, tmp_path): +@pytest.fixture(name="mock_addons_git", autouse=True) +async def fixture_mock_addons_git(tmp_supervisor_data: Path) -> None: + """Mock addons git path.""" + with patch.object( + CoreConfig, + "path_addons_git", + new=PropertyMock(return_value=tmp_supervisor_data / "addons" / "git"), + ): + yield + + +def add_store_reset_suggestion(coresys: CoreSys) -> None: + """Add suggestion for tests.""" + coresys.resolution.add_suggestion( + Suggestion( + SuggestionType.EXECUTE_RESET, ContextType.STORE, reference="94cfad5a" + ) + ) + coresys.resolution.add_issue( + Issue(IssueType.CORRUPT_REPOSITORY, ContextType.STORE, reference="94cfad5a") + ) + + +@pytest.mark.usefixtures("supervisor_internet") +async def test_fixup(coresys: CoreSys): """Test fixup.""" store_execute_reset = FixupStoreExecuteReset(coresys) - test_repo = Path(tmp_path, "test_repo") + test_repo = coresys.config.path_addons_git / "94cfad5a" assert store_execute_reset.auto - coresys.resolution.add_suggestion( - Suggestion(SuggestionType.EXECUTE_RESET, ContextType.STORE, reference="test") - ) - coresys.resolution.add_issue( - Issue(IssueType.CORRUPT_REPOSITORY, ContextType.STORE, reference="test") - ) - - test_repo.mkdir() - (test_repo / ".git").mkdir() + add_store_reset_suggestion(coresys) + test_repo.mkdir(parents=True) + good_marker = test_repo / ".git" + (corrupt_marker := (test_repo / "corrupt")).touch() assert test_repo.exists() + assert not good_marker.exists() + assert corrupt_marker.exists() - mock_repositorie = AsyncMock() - mock_repositorie.git.path = test_repo - coresys.store.repositories["test"] = mock_repositorie - assert len(listdir(test_repo)) > 0 + async def mock_clone(obj: GitRepo, path: Path | None = None): + """Mock of clone method.""" + path = path or obj.path + await coresys.run_in_executor((path / ".git").mkdir) - with patch("shutil.disk_usage", return_value=(42, 42, 2 * (1024.0**3))): + coresys.store.repositories["94cfad5a"] = Repository( + coresys, "https://github.com/home-assistant/addons-example" + ) + with ( + patch.object(GitRepo, "load"), + patch.object(GitRepo, "_clone", new=mock_clone), + patch("shutil.disk_usage", return_value=(42, 42, 2 * (1024.0**3))), + ): await store_execute_reset() - assert len(listdir(test_repo)) == 0 - assert mock_repositorie.load.called + assert test_repo.exists() + assert good_marker.exists() + assert not corrupt_marker.exists() assert len(coresys.resolution.suggestions) == 0 assert len(coresys.resolution.issues) == 0 + assert len(listdir(coresys.config.path_tmp)) == 0 + + +@pytest.mark.usefixtures("supervisor_internet") +async def test_fixup_clone_fail(coresys: CoreSys): + """Test fixup does not delete cache when clone fails.""" + store_execute_reset = FixupStoreExecuteReset(coresys) + test_repo = coresys.config.path_addons_git / "94cfad5a" + + add_store_reset_suggestion(coresys) + test_repo.mkdir(parents=True) + (corrupt_marker := (test_repo / "corrupt")).touch() + assert test_repo.exists() + assert corrupt_marker.exists() + + coresys.store.repositories["94cfad5a"] = Repository( + coresys, "https://github.com/home-assistant/addons-example" + ) + with ( + patch.object(GitRepo, "load"), + patch.object(GitRepo, "_clone", side_effect=StoreGitCloneError), + patch("shutil.disk_usage", return_value=(42, 42, 2 * (1024.0**3))), + ): + await store_execute_reset() + + assert test_repo.exists() + assert corrupt_marker.exists() + assert len(coresys.resolution.suggestions) == 1 + assert len(coresys.resolution.issues) == 1 + assert len(listdir(coresys.config.path_tmp)) == 0 + + +@pytest.mark.parametrize( + ("error_num", "unhealthy"), [(errno.EBUSY, False), (errno.EBADMSG, True)] +) +@pytest.mark.usefixtures("supervisor_internet") +async def test_fixup_move_fail(coresys: CoreSys, error_num: int, unhealthy: bool): + """Test fixup cleans up clone on move fail. + + This scenario shouldn't really happen unless something is pretty wrong with the system. + It will leave the user in a bind without the git cache but at least we try to clean up tmp. + """ + store_execute_reset = FixupStoreExecuteReset(coresys) + test_repo = coresys.config.path_addons_git / "94cfad5a" + + add_store_reset_suggestion(coresys) + test_repo.mkdir(parents=True) + coresys.store.repositories["94cfad5a"] = Repository( + coresys, "https://github.com/home-assistant/addons-example" + ) + with ( + patch.object(GitRepo, "load"), + patch.object(GitRepo, "_clone"), + patch("supervisor.store.git.Path.rename", side_effect=(err := OSError())), + patch("shutil.disk_usage", return_value=(42, 42, 2 * (1024.0**3))), + ): + err.errno = error_num + await store_execute_reset() + + assert len(coresys.resolution.suggestions) == 1 + assert len(coresys.resolution.issues) == 1 + assert len(listdir(coresys.config.path_tmp)) == 0 + assert ( + UnhealthyReason.OSERROR_BAD_MESSAGE in coresys.resolution.unhealthy + ) is unhealthy