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

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

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 = {
+            await self._clone()
-                attribute: value
-                for attribute, value in (
-                    ("recursive", True),
-                    ("branch", self.branch),
-                    ("depth", 1),
-                    ("shallow-submodules", True),
-                )
-                if value is not None
-            }
 
-            try:
+    @Job(
-                _LOGGER.info(
+        name="git_repo_reset",
-                    "Cloning add-on %s repository from %s", self.path, self.url
+        conditions=[JobCondition.FREE_SPACE, JobCondition.INTERNET_SYSTEM],
-                )
+        on_condition=StoreJobError,
-                self.repo = await self.sys_run_in_executor(
+    )
-                    ft.partial(
+    async def reset(self) -> None:
-                        git.Repo.clone_from,
+        """Reset repository to fix issue with local copy."""
-                        self.url,
+        # Clone into temporary folder
-                        str(self.path),
+        temp_dir = await self.sys_run_in_executor(
-                        **git_args,  # type: ignore
+            TemporaryDirectory, dir=self.sys_config.path_tmp
-                    )
+        )
-                )
+        temp_path = Path(temp_dir.name)
+        try:
+            await self._clone(temp_path)
 
-            except (
+            # Remove corrupted repo and move temp clone to its place
-                git.InvalidGitRepositoryError,
+            def move_clone():
-                git.NoSuchPathError,
+                remove_folder(folder=self.path)
-                git.CommandError,
+                temp_path.rename(self.path)
-                UnicodeDecodeError,
+
-            ) as err:
+            async with self.lock:
-                _LOGGER.error("Can't clone %s repository: %s.", self.url, err)
+                try:
-                raise StoreGitCloneError() from err
+                    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",

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(
+    add_store_reset_suggestion(coresys)
-        Suggestion(SuggestionType.EXECUTE_RESET, ContextType.STORE, reference="test")
+    test_repo.mkdir(parents=True)
-    )
+    good_marker = test_repo / ".git"
-    coresys.resolution.add_issue(
+    (corrupt_marker := (test_repo / "corrupt")).touch()
-        Issue(IssueType.CORRUPT_REPOSITORY, ContextType.STORE, reference="test")
-    )
-
-    test_repo.mkdir()
-    (test_repo / ".git").mkdir()
     assert test_repo.exists()
+    assert not good_marker.exists()
+    assert corrupt_marker.exists()
 
-    mock_repositorie = AsyncMock()
+    async def mock_clone(obj: GitRepo, path: Path | None = None):
-    mock_repositorie.git.path = test_repo
+        """Mock of clone method."""
-    coresys.store.repositories["test"] = mock_repositorie
+        path = path or obj.path
-    assert len(listdir(test_repo)) > 0
+        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 test_repo.exists()
-    assert mock_repositorie.load.called
+    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