diff --git a/AddonCatalog.py b/AddonCatalog.py
index fe960a1a..c7ceec66 100644
--- a/AddonCatalog.py
+++ b/AddonCatalog.py
@@ -89,6 +89,7 @@ class AddonCatalogEntry:
relative_cache_path: str = "" # Generated by the cache system
git_hash: Optional[str] = None # Generated by the cache system
git_tag: Optional[str] = None # Generated by the cache system
+ cache_error: Optional[str] = None # Generated by the cache system
def __init__(self, raw_data: Dict[str, str]) -> None:
"""Create an AddonDictionaryEntry from the raw JSON data"""
@@ -360,6 +361,15 @@ def add_git_info_to_entry(
entry.git_hash = commit_hash
entry.git_tag = tag
+ def add_cache_error_to_entry(self, addon_id: str, index: int, error: Optional[str]) -> None:
+ """Records why this entry's cache generation failed, if it did."""
+ entries = self._dictionary.get(addon_id)
+ if entries is None:
+ raise RuntimeError(f"Addon {addon_id} does not exist")
+ if index >= len(entries):
+ raise RuntimeError(f"Addon {addon_id} index out of range")
+ entries[index].cache_error = error
+
def get_available_branches(self, addon_id: str) -> List[str]:
"""For a given ID, get the list of available branches compatible with this version of
FreeCAD.
diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py
index 36e0bed2..bde9347a 100644
--- a/AddonCatalogCacheCreator.py
+++ b/AddonCatalogCacheCreator.py
@@ -23,41 +23,40 @@
Intended to be run by a server-side systemd timer to generate a file that is then loaded by the
Addon Manager in each FreeCAD installation."""
-import datetime
-import shutil
-import sys
-from dataclasses import is_dataclass, fields
-from typing import Any, List, Optional, Dict, Tuple
-
import base64
+import datetime
import enum
import hashlib
import io
import json
import os
import re
-import requests
+import shutil
# Audited: all subprocess calls in this module are fixed git argument lists run with no shell;
# the variable arguments (url, branch, name) come from the addon index this tool exists to
# process (added nosec B404, and B603/B607 at the call sites)
import subprocess # nosec B404
-from typing import List
+import sys
+import time
+import traceback
+import zipfile
+from dataclasses import fields, is_dataclass
+from types import SimpleNamespace
+from typing import Any, Dict, List, Optional, Set, Tuple
# Audited: only the exception class is imported, for catching errors raised by defusedxml,
# which re-exports this same class. All parsing is done by defusedxml. (added nosec B405)
from xml.etree.ElementTree import ParseError as XmlParseError # nosec B405
+import requests
from defusedxml import DefusedXmlException
-import zipfile
+from scour import scour
import AddonCatalog
+import addonmanager_icon_utilities as icon_utils
import addonmanager_metadata
import addonmanager_utilities as utils
-import addonmanager_icon_utilities as icon_utils
-
-from scour import scour
-from types import SimpleNamespace
ADDON_CATALOG_URL = "https://raw.githubusercontent.com/FreeCAD/Addons/main/Data/Index.json"
BASE_DIRECTORY = "./CatalogCache"
@@ -65,6 +64,8 @@
CLONE_TIMEOUT = (
300 # Seconds: repos that take longer than this are assumed to be too large to index
)
+MAX_ATTEMPTS = 3 # Attempts before giving up on a single git or HTTP operation
+RETRY_DELAY_SECONDS = 5 # Delay between retry attempts of a failed git or HTTP operation
def recursive_serialize(obj: Any):
@@ -127,6 +128,20 @@ def __init__(self):
self._cache = {}
self._sanitize_counter = 0
self._directory_name_cache: Dict[str, str] = {}
+ self._previously_failed_addon_ids: Set[str] = set()
+
+ def _load_previously_failed_addon_ids(self) -> Set[str]:
+ """If any previous clone errors are found, return the set of addon IDs
+ that failed to clone/update or download."""
+ path = os.path.join(self.cwd, "clone_errors.json")
+ if not os.path.isfile(path):
+ return set()
+ try:
+ with open(path, "r", encoding="utf-8") as f:
+ previous_errors = json.load(f)
+ except (OSError, json.JSONDecodeError):
+ return set()
+ return {dirname.split(os.sep, 1)[0] for dirname in previous_errors}
def write(self, addon_id: Optional[str] = None) -> None:
original_working_directory = os.getcwd()
@@ -134,6 +149,7 @@ def write(self, addon_id: Optional[str] = None) -> None:
os.chdir(self.cwd)
try:
+ self._previously_failed_addon_ids = self._load_previously_failed_addon_ids()
fetcher = CatalogFetcher()
self.catalog = fetcher.catalog
@@ -202,8 +218,12 @@ def write(self, addon_id: Optional[str] = None) -> None:
os.chdir(original_working_directory)
def create_local_copy_of_addons(self):
+ # Addons that failed last run are retried first, so a rate-limit-induced failure doesn't
+ # always strand the same addons at the tail end of a fixed processing order.
+ catalog_items = list(self.catalog.get_catalog().items())
+ catalog_items.sort(key=lambda item: item[0] not in self._previously_failed_addon_ids)
counter = 0
- for addon_id, catalog_entries in self.catalog.get_catalog().items():
+ for addon_id, catalog_entries in catalog_items:
self.create_local_copy_of_single_addon(addon_id, catalog_entries)
counter += 1
if counter >= MAX_COUNT:
@@ -228,6 +248,9 @@ def create_local_copy_of_single_addon(
"Neither git info nor zip info was specified."
)
continue
+ dirname = self.get_directory_name(addon_id, index, catalog_entry)
+ if dirname in self.clone_errors:
+ self.catalog.add_cache_error_to_entry(addon_id, index, self.clone_errors[dirname])
metadata = self.generate_cache_entry(addon_id, index, catalog_entry)
self.catalog.add_metadata_to_entry(addon_id, index, metadata)
git_hash, git_tag = self.get_git_info(addon_id, index, catalog_entry)
@@ -476,65 +499,137 @@ def get_directory_name(self, addon_id, index, catalog_entry):
def create_local_copy_of_single_addon_with_zip(
self, addon_id: str, index: int, catalog_entry: AddonCatalog.AddonCatalogEntry
):
- response = requests.get(catalog_entry.zip_url, timeout=10.0)
- if response.status_code != 200:
- print(f"ERROR: Failed to fetch zip data for {addon_id} from {catalog_entry.zip_url}.")
- return
extract_to_dir = self.get_directory_name(addon_id, index, catalog_entry)
+ response = None
+ last_error_message = "unknown error"
+ for attempt in range(1, MAX_ATTEMPTS + 1):
+ try:
+ response = requests.get(catalog_entry.zip_url, timeout=10.0)
+ except requests.exceptions.RequestException as e:
+ last_error_message = (
+ f"Network error fetching {catalog_entry.zip_url}: {e}\n{traceback.format_exc()}"
+ )
+ response = None
+ else:
+ if response.status_code == 200:
+ break
+ last_error_message = (
+ f"Failed to fetch zip data for {addon_id} from {catalog_entry.zip_url}: "
+ f"HTTP {response.status_code}"
+ )
+ response = None
+ print(f"WARNING: {last_error_message} (attempt {attempt}/{MAX_ATTEMPTS})", flush=True)
+ if attempt < MAX_ATTEMPTS:
+ time.sleep(RETRY_DELAY_SECONDS)
+
+ if response is None:
+ error_message = f"{last_error_message}\nafter {MAX_ATTEMPTS} attempts"
+ print(f"ERROR: {error_message}")
+ self.clone_errors[extract_to_dir] = error_message
+ return
+
if os.path.exists(extract_to_dir):
utils.rmdir(extract_to_dir)
os.makedirs(extract_to_dir, exist_ok=True)
- with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file:
- latest = max(
- (info.date_time for info in zip_file.infolist() if not info.is_dir()), default=None
+ try:
+ with zipfile.ZipFile(io.BytesIO(response.content)) as zip_file:
+ latest = max(
+ (info.date_time for info in zip_file.infolist() if not info.is_dir()),
+ default=None,
+ )
+ if latest is not None:
+ catalog_entry.last_update_time = datetime.datetime(*latest).isoformat()
+ zip_file.extractall(path=extract_to_dir)
+ except (zipfile.BadZipFile, OSError) as e:
+ error_message = (
+ f"Downloaded zip data for {addon_id} from {catalog_entry.zip_url} is invalid: "
+ f"{e}\n{traceback.format_exc()}"
)
- if latest is not None:
- catalog_entry.last_update_time = datetime.datetime(*latest).isoformat()
- zip_file.extractall(path=extract_to_dir)
+ print(f"ERROR: {error_message}")
+ self.clone_errors[extract_to_dir] = error_message
+
+ @staticmethod
+ def _tail(text: object, limit: int = 2000) -> str:
+ """Return the trailing portion of some text so that one addon's error can't
+ bloat the shipped cache. The end of the text is kept because that's where
+ git and pip put their actual "fatal: ..." error line."""
+ if not isinstance(text, str) or not text.strip():
+ return ""
+ stripped = text.strip()
+ return stripped if len(stripped) <= limit else "…" + stripped[-limit:]
+
+ def clone_with_retries(self, url: str, branch: str, target_dir: str) -> None:
+ """Attempt a shallow 'git clone' of url/branch into target_dir, retrying up to
+ MAX_ATTEMPTS times."""
+ # Shallow, but do include the last commit on each branch and tag
+ command = ["git", "clone", "--depth", "1", "--branch", branch, url, target_dir]
+ last_error_message = "unknown error"
+ for attempt in range(1, MAX_ATTEMPTS + 1):
+ if os.path.exists(target_dir):
+ utils.rmdir(target_dir)
+ print(f"Cloning {url} to {target_dir}", flush=True)
+ try:
+ completed_process = subprocess.run( # nosec B603
+ command, timeout=CLONE_TIMEOUT, capture_output=True, text=True
+ )
+ except subprocess.TimeoutExpired as e:
+ summary = f"Clone of {url} timed out after {CLONE_TIMEOUT} seconds"
+ detail = self._tail(e.stderr)
+ else:
+ if completed_process.returncode == 0:
+ return
+ summary = f"Failed to clone {url}: git exited with {completed_process.returncode}"
+ detail = self._tail(completed_process.stderr)
+ last_error_message = f"{summary}\n{detail}" if detail else summary
+ print(f"WARNING: {last_error_message} (attempt {attempt}/{MAX_ATTEMPTS})", flush=True)
+ if attempt < MAX_ATTEMPTS:
+ time.sleep(RETRY_DELAY_SECONDS)
+ if os.path.exists(target_dir):
+ utils.rmdir(target_dir)
+ raise RuntimeError(f"{last_error_message}\nafter {MAX_ATTEMPTS} attempts")
def clone_or_update(self, name: str, url: str, branch: str) -> None:
"""If a directory called "name" exists, and it contains a subdirectory called .git,
then the local copy is fetched and hard reset onto the requested ref; otherwise we use
- 'git clone' to make a shallow copy of the repo."""
+ 'git clone' to make a shallow copy of the repo. Transient failures are retried before
+ giving up. If updating an existing copy fails every attempt, it is left untouched (not
+ deleted), so this cycle's cache generation still finds whatever good data it already
+ had from a previous run."""
if not os.path.exists(os.path.join(os.getcwd(), name, ".git")):
- print(f"Cloning {url} to {name}", flush=True)
- # Shallow, but do include the last commit on each branch and tag
- command = [
- "git",
- "clone",
- "--depth",
- "1",
- "--branch",
- branch,
- url,
- name,
- ]
- try:
- completed_process = subprocess.run(command, timeout=CLONE_TIMEOUT) # nosec B603
- except subprocess.TimeoutExpired:
- self.clone_errors[name] = f"Timed out after {CLONE_TIMEOUT} seconds."
- raise RuntimeError(f"Clone of {url} timed out.")
- # TODO: Automatically fall back to a sparse clone
- if completed_process.returncode != 0:
- self.clone_errors[name] = f"Failed to clone {url}: {completed_process.returncode}"
- raise RuntimeError(f"Clone failed for {url}")
- else:
- print(f"Updating {name}", flush=True)
- old_dir = os.getcwd()
- os.chdir(os.path.join(old_dir, name))
try:
- CacheWriter.fetch_and_reset(name, url, branch)
+ self.clone_with_retries(url, branch, name)
except RuntimeError as e:
- # In the event of basically ANY error, delete the original and re-clone.
- print(e)
- print("Deleting and re-cloning the original repo")
- os.chdir(old_dir)
- utils.rmdir(os.path.join(old_dir, name))
- self.clone_or_update(name, url, branch)
+ self.clone_errors[name] = str(e)
+ raise
+ return
+
+ print(f"Updating {name}", flush=True)
+ old_dir = os.getcwd()
+ os.chdir(os.path.join(old_dir, name))
+ last_error: Optional[RuntimeError] = None
+ try:
+ for attempt in range(1, MAX_ATTEMPTS + 1):
+ try:
+ CacheWriter.fetch_and_reset(name, url, branch)
+ last_error = None
+ break
+ except RuntimeError as e:
+ last_error = e
+ print(
+ f"WARNING: Update attempt {attempt}/{MAX_ATTEMPTS} failed for {name}: {e}",
+ flush=True,
+ )
+ if attempt < MAX_ATTEMPTS:
+ time.sleep(RETRY_DELAY_SECONDS)
+ finally:
os.chdir(old_dir)
+ if last_error is not None:
+ self.clone_errors[name] = str(last_error)
+ raise last_error
+
def sparse_clone(self, name: str, url: str, branch: str, files: List[str]) -> None:
"""Perform a sparse clone of a git repo, including only the specified files. Overwrite any
existing path."""
@@ -635,32 +730,42 @@ def get_icon_from_metadata(metadata: addonmanager_metadata.Metadata) -> Optional
@staticmethod
def fetch_and_reset(name: str, url: str, branch: str) -> None:
"""Update the git clone in the current working directory by fetching from its remote and
- hard resetting onto the requested ref, discarding any local state. A RuntimeError is raised
- if any of the git calls fails."""
+ hard resetting onto the requested ref, discarding any local state. A RuntimeError, with
+ git's own stderr appended if any was captured, is raised if any of the git calls fails."""
try:
completed_process = subprocess.run( # nosec B603 B607
- ["git", "fetch", "--force"], timeout=CLONE_TIMEOUT
+ ["git", "fetch", "--force"], timeout=CLONE_TIMEOUT, capture_output=True, text=True
+ )
+ except subprocess.TimeoutExpired as e:
+ raise RuntimeError(
+ f"git fetch for {name} timed out after {CLONE_TIMEOUT} seconds. "
+ f"{CacheWriter._tail(e.stderr)}".strip()
)
- except subprocess.TimeoutExpired:
- raise RuntimeError(f"git fetch for {name} timed out after {CLONE_TIMEOUT} seconds")
if completed_process.returncode != 0:
- raise RuntimeError(f"git fetch failed for {name}")
+ raise RuntimeError(
+ f"git fetch failed for {name}. {CacheWriter._tail(completed_process.stderr)}".strip()
+ )
git_ref_type = CacheWriter.determine_git_ref_type(name, url, branch)
reset_target = f"origin/{branch}" if git_ref_type == GitRefType.BRANCH else branch
completed_process = subprocess.run( # nosec B603 B607
- ["git", "reset", "--hard", reset_target, "--quiet"]
+ ["git", "reset", "--hard", reset_target, "--quiet"], capture_output=True, text=True
)
if completed_process.returncode != 0:
- raise RuntimeError(f"git reset failed for {name} ref {reset_target}")
+ raise RuntimeError(
+ f"git reset failed for {name} ref {reset_target}. "
+ f"{CacheWriter._tail(completed_process.stderr)}".strip()
+ )
completed_process = subprocess.run( # nosec B603 B607
- ["git", "clean", "-x", "-f", "-d", "--quiet"]
+ ["git", "clean", "-x", "-f", "-d", "--quiet"], capture_output=True, text=True
)
if completed_process.returncode != 0:
- raise RuntimeError(f"git clean failed for {name}")
+ raise RuntimeError(
+ f"git clean failed for {name}. {CacheWriter._tail(completed_process.stderr)}".strip()
+ )
@staticmethod
def determine_git_ref_type(name: str, _url: str, branch: str) -> GitRefType:
diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py
index 6ea28e41..f9b60b7c 100644
--- a/AddonManagerTest/app/test_addon_catalog_cache_creator.py
+++ b/AddonManagerTest/app/test_addon_catalog_cache_creator.py
@@ -24,20 +24,17 @@
import base64
import dataclasses
+import os
from unittest import mock
+from unittest.mock import MagicMock, patch
from pyfakefs.fake_filesystem_unittest import TestCase
-from unittest.mock import patch, MagicMock
-
-import os
-
-import AddonCatalogCacheCreator as accc
import AddonCatalog
+import AddonCatalogCacheCreator as accc
class TestRecursiveSerialize(TestCase):
-
def test_simple_object(self):
result = accc.recursive_serialize("just a string")
self.assertEqual(result, "just a string")
@@ -109,7 +106,6 @@ def test_real_catalog(self):
class TestCacheWriter(TestCase):
-
def setUp(self):
self.setUpPyfakefs()
@@ -323,7 +319,11 @@ def test_create_local_copy_of_single_addon_with_impossible_sparse_clone(
marked as sparse: clients must not be told to look for a zip that does not exist."""
catalog_entries = [
AddonCatalog.AddonCatalogEntry(
- {"repository": "https://some.url", "git_ref": "main", "sparse_cache": True}
+ {
+ "repository": "https://some.url",
+ "git_ref": "main",
+ "sparse_cache": True,
+ }
),
]
writer = accc.CacheWriter()
@@ -347,7 +347,11 @@ def test_create_local_copy_of_single_addon_using_git(self, mock_create_with_git)
{"repository": "https://some.url", "git_ref": "branch-2"}
),
AddonCatalog.AddonCatalogEntry(
- {"repository": "https://some.url", "git_ref": "branch-3", "zip_url": "zip"}
+ {
+ "repository": "https://some.url",
+ "git_ref": "branch-3",
+ "zip_url": "zip",
+ }
),
]
writer = accc.CacheWriter()
@@ -367,7 +371,11 @@ def test_create_local_copy_of_single_addon_using_zip(
AddonCatalog.AddonCatalogEntry({"zip_url": "zip1"}),
AddonCatalog.AddonCatalogEntry({"zip_url": "zip2"}),
AddonCatalog.AddonCatalogEntry(
- {"repository": "https://some.url", "git_ref": "branch-3", "zip_url": "zip3"}
+ {
+ "repository": "https://some.url",
+ "git_ref": "branch-3",
+ "zip_url": "zip3",
+ }
),
]
writer = accc.CacheWriter()
@@ -415,6 +423,29 @@ def get_catalog(self):
mock_create_single_addon.assert_any_call("TestMod2", mock.ANY)
self.assertEqual(3, mock_create_single_addon.call_count)
+ @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon")
+ def test_create_local_copy_of_addons_processes_previously_failed_addons_first(
+ self, mock_create_single_addon
+ ):
+ """Addons that failed last run are retried before working through the rest, in case the
+ failure was caused by rate limiting partway through the previous run."""
+
+ class MockCatalog:
+ def get_catalog(self):
+ return {
+ "TestMod1": [AddonCatalog.AddonCatalogEntry({"git_ref": "main"})],
+ "TestMod2": [AddonCatalog.AddonCatalogEntry({"git_ref": "main"})],
+ "TestMod3": [AddonCatalog.AddonCatalogEntry({"git_ref": "main"})],
+ "TestMod4": [AddonCatalog.AddonCatalogEntry({"git_ref": "main"})],
+ }
+
+ writer = accc.CacheWriter()
+ writer.catalog = MockCatalog()
+ writer._previously_failed_addon_ids = {"TestMod3"}
+ writer.create_local_copy_of_addons()
+ processed_order = [call.args[0] for call in mock_create_single_addon.call_args_list]
+ self.assertEqual(["TestMod3", "TestMod1", "TestMod2", "TestMod4"], processed_order)
+
class TestCacheWriterGitUpdate(TestCase):
"""Tests of the git commands used to bring an existing local clone up to date."""
@@ -445,7 +476,10 @@ def test_fetch_and_reset_with_tag(self, mock_ref_type, mock_run):
mock_ref_type.return_value = accc.GitRefType.TAG
mock_run.return_value.returncode = 0
accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "v1.0")
- self.assertIn(["git", "reset", "--hard", "v1.0", "--quiet"], self.issued_commands(mock_run))
+ self.assertIn(
+ ["git", "reset", "--hard", "v1.0", "--quiet"],
+ self.issued_commands(mock_run),
+ )
@patch("AddonCatalogCacheCreator.subprocess.run")
@patch("AddonCatalogCacheCreator.CacheWriter.determine_git_ref_type")
@@ -455,7 +489,8 @@ def test_fetch_and_reset_with_hash(self, mock_ref_type, mock_run):
mock_run.return_value.returncode = 0
accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "abc123")
self.assertIn(
- ["git", "reset", "--hard", "abc123", "--quiet"], self.issued_commands(mock_run)
+ ["git", "reset", "--hard", "abc123", "--quiet"],
+ self.issued_commands(mock_run),
)
@patch("AddonCatalogCacheCreator.subprocess.run")
@@ -476,7 +511,10 @@ def test_fetch_and_reset_removes_untracked_files(self, mock_ref_type, mock_run):
mock_ref_type.return_value = accc.GitRefType.BRANCH
mock_run.return_value.returncode = 0
accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main")
- self.assertIn(["git", "clean", "-x", "-f", "-d", "--quiet"], self.issued_commands(mock_run))
+ self.assertIn(
+ ["git", "clean", "-x", "-f", "-d", "--quiet"],
+ self.issued_commands(mock_run),
+ )
@patch("AddonCatalogCacheCreator.subprocess.run")
def test_fetch_and_reset_raises_when_fetch_fails(self, mock_run):
@@ -501,18 +539,104 @@ def test_fetch_and_reset_raises_when_reset_fails(self, mock_ref_type, mock_run):
with self.assertRaises(RuntimeError):
accc.CacheWriter.fetch_and_reset("TestMod", "https://some.url", "main")
+ @patch("AddonCatalogCacheCreator.time.sleep")
@patch("AddonCatalogCacheCreator.subprocess.run")
- @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset")
- def test_clone_or_update_reclones_when_update_fails(self, mock_update, mock_run):
- """If the update fails, the local copy is deleted and cloned again."""
- mock_update.side_effect = RuntimeError("Update failed")
+ def test_clone_or_update_raises_after_exhausting_clone_attempts(self, mock_run, mock_sleep):
+ """If every attempt fails, the caller is told, with a reason recorded for this addon."""
+ mock_run.return_value.returncode = 1
+ writer = accc.CacheWriter()
+ with self.assertRaises(RuntimeError):
+ writer.clone_or_update("TestMod", "https://some.url", "main")
+ self.assertEqual(accc.MAX_ATTEMPTS, mock_run.call_count)
+ self.assertEqual(accc.MAX_ATTEMPTS - 1, mock_sleep.call_count)
+ self.assertIn("TestMod", writer.clone_errors)
+
+ @patch("AddonCatalogCacheCreator.utils.rmdir")
+ @patch("AddonCatalogCacheCreator.subprocess.run")
+ def test_clone_or_update_removes_leftover_directory_before_clone_attempt(
+ self, mock_run, mock_rmdir
+ ):
+ """A partial checkout left by an earlier killed attempt doesn't block a clean retry."""
mock_run.return_value.returncode = 0
+ self.fake_fs().create_dir(os.path.join(os.getcwd(), "TestMod", "partial-checkout"))
+ writer = accc.CacheWriter()
+ writer.clone_or_update("TestMod", "https://some.url", "main")
+ mock_rmdir.assert_any_call("TestMod")
+
+ @patch("AddonCatalogCacheCreator.time.sleep")
+ @patch("AddonCatalogCacheCreator.subprocess.run")
+ @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset")
+ def test_clone_or_update_retries_update_before_reclone(
+ self, mock_fetch_and_reset, mock_run, mock_sleep
+ ):
+ """A single transient update failure is retried and self-heals: no deletion, no reclone."""
+ mock_fetch_and_reset.side_effect = [RuntimeError("transient"), None]
clone_path = os.path.join(os.getcwd(), "TestMod")
self.fake_fs().create_dir(os.path.join(clone_path, ".git"))
writer = accc.CacheWriter()
writer.clone_or_update("TestMod", "https://some.url", "main")
- self.assertFalse(os.path.exists(clone_path))
- self.assertIn("clone", self.issued_commands(mock_run)[0])
+ self.assertEqual(2, mock_fetch_and_reset.call_count)
+ self.assertEqual(1, mock_sleep.call_count)
+ self.assertEqual(0, mock_run.call_count)
+ self.assertTrue(os.path.exists(clone_path))
+ self.assertEqual({}, writer.clone_errors)
+
+ @patch("AddonCatalogCacheCreator.time.sleep")
+ @patch("AddonCatalogCacheCreator.subprocess.run")
+ @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset")
+ def test_clone_or_update_reclones_after_exhausting_update_retries(
+ self, mock_fetch_and_reset, mock_run, mock_sleep
+ ):
+ """If every update attempt fails, a fresh clone into a temp dir replaces the original."""
+ mock_fetch_and_reset.side_effect = RuntimeError("persistent failure")
+
+ def fake_clone(command, timeout=None): # pylint: disable=unused-argument
+ self.fake_fs().create_dir(os.path.join(command[-1], ".git"))
+ return MagicMock(returncode=0)
+
+ mock_run.side_effect = fake_clone
+ clone_path = os.path.join(os.getcwd(), "TestMod")
+ self.fake_fs().create_file(
+ os.path.join(clone_path, ".git", "HEAD"), contents="ref: refs/heads/main\n"
+ )
+ writer = accc.CacheWriter()
+ writer.clone_or_update("TestMod", "https://some.url", "main")
+ self.assertEqual(accc.MAX_ATTEMPTS, mock_fetch_and_reset.call_count)
+ commands = self.issued_commands(mock_run)
+ self.assertTrue(
+ any("clone" in cmd and cmd[-1] == "TestMod.reclone-tmp" for cmd in commands)
+ )
+ self.assertTrue(os.path.isdir(os.path.join("TestMod", ".git")))
+ self.assertFalse(os.path.exists("TestMod.reclone-tmp"))
+ self.assertFalse(os.path.exists("TestMod.reclone-old"))
+ self.assertNotIn("TestMod", writer.clone_errors)
+
+ @patch("AddonCatalogCacheCreator.time.sleep")
+ @patch("AddonCatalogCacheCreator.subprocess.run")
+ @patch("AddonCatalogCacheCreator.CacheWriter.fetch_and_reset")
+ def test_clone_or_update_reclone_failure_leaves_original_directory_untouched(
+ self, mock_fetch_and_reset, mock_run, mock_sleep
+ ):
+ """If the fallback re-clone also fails, the existing good copy is left exactly as-is."""
+ mock_fetch_and_reset.side_effect = RuntimeError("persistent failure")
+ mock_run.return_value.returncode = 1 # The fallback re-clone fails every attempt too.
+ clone_path = os.path.join(os.getcwd(), "TestMod")
+ self.fake_fs().create_file(
+ os.path.join(clone_path, ".git", "HEAD"), contents="ref: refs/heads/main\n"
+ )
+ self.fake_fs().create_file(
+ os.path.join(clone_path, "package.xml"),
+ contents="marker",
+ )
+ writer = accc.CacheWriter()
+ with self.assertRaises(RuntimeError):
+ writer.clone_or_update("TestMod", "https://some.url", "main")
+ self.assertEqual(accc.MAX_ATTEMPTS, mock_fetch_and_reset.call_count)
+ self.assertTrue(os.path.isdir(clone_path))
+ with open(os.path.join(clone_path, "package.xml"), encoding="utf-8") as f:
+ self.assertEqual("marker", f.read())
+ self.assertIn("TestMod", writer.clone_errors)
+ self.assertFalse(os.path.exists("TestMod.reclone-tmp"))
@patch("AddonCatalogCacheCreator.subprocess.run")
def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run):
diff --git a/AddonManagerTest/app/test_addoncatalog.py b/AddonManagerTest/app/test_addoncatalog.py
index 1d0cb020..91c9a140 100644
--- a/AddonManagerTest/app/test_addoncatalog.py
+++ b/AddonManagerTest/app/test_addoncatalog.py
@@ -6,7 +6,7 @@
"""Tests for the AddonCatalog and AddonCatalogEntry classes."""
-from unittest import mock, main, TestCase
+from unittest import TestCase, main, mock
from unittest.mock import patch
AddonCatalogEntry = None
@@ -23,7 +23,7 @@ def setUp(self):
"""Start mock for addonmanager_licenses class."""
self.addon_patch = mock.patch.dict("sys.modules", {"addonmanager_licenses": mock.Mock()})
self.mock_addon_module = self.addon_patch.start()
- from AddonCatalog import AddonCatalogEntry, AddonCatalog
+ from AddonCatalog import AddonCatalog, AddonCatalogEntry
from addonmanager_metadata import Version
self.AddonCatalogEntry = AddonCatalogEntry
@@ -144,6 +144,17 @@ def test_instantiate_addon_with_sparse_cache_and_no_zip(self):
with self.assertRaises(RuntimeError):
ac.instantiate_addon("parts_library")
+ def test_cache_error_field_round_trips_from_raw_data(self):
+ """The cache_error field, generated by the cache system, is accepted like any other
+ recognized raw-dict key."""
+ ac = self.AddonCatalogEntry({"cache_error": "Clone failed for https://example.test"})
+ self.assertEqual("Clone failed for https://example.test", ac.cache_error)
+
+ def test_cache_error_defaults_to_none(self):
+ """An entry with no cache_error key is assumed to have generated cleanly."""
+ ac = self.AddonCatalogEntry({})
+ self.assertIsNone(ac.cache_error)
+
def test_instantiate_addon_with_zip_only(self):
"""An Addon with no repository is downloaded from the catalog's zip."""
ac = self.AddonCatalogEntry({"zip_url": "https://example.com/an_addon.zip"})
@@ -417,6 +428,20 @@ def test_get_addon_from_id_with_icon_data(self):
addon = self._get_addon_for_test(metadata_dict)
self.assertIsNotNone(addon.icon_data)
+ def test_add_cache_error_to_entry_sets_the_field(self):
+ """The cache creator can record why an entry's cache generation failed."""
+ data = {"AnAddon": [{"git_ref": "main"}]}
+ catalog = self.AddonCatalog(data)
+ catalog.add_cache_error_to_entry("AnAddon", 0, "Clone failed after 3 attempts")
+ entry = catalog.get_catalog()["AnAddon"][0]
+ self.assertEqual("Clone failed after 3 attempts", entry.cache_error)
+
+ def test_add_cache_error_to_entry_unknown_addon_raises(self):
+ """Recording a cache error for an addon ID that doesn't exist is a programming error."""
+ catalog = self.AddonCatalog({"AnAddon": [{"git_ref": "main"}]})
+ with self.assertRaises(RuntimeError):
+ catalog.add_cache_error_to_entry("NoSuchAddon", 0, "boom")
+
def _get_addon_for_test(self, metadata_dict) -> Addon:
metadata = self.CatalogEntryMetadata.from_dict(metadata_dict)
data = {