From 579e8d47d14da00c96aaefe1e44cb6038ff7439f Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Thu, 3 Sep 2026 13:19:52 +0200 Subject: [PATCH 1/4] ENH: add retry mecanism to the addon cache builder A crash during the 6-hourly cache rebuild could permanently drop an addon's cached package.xml/icon for that cycle (since the existing good clone was deleted beforehand). Retry git commands a few times before giving up. If an update ultimately fails, keep the existing good clone. Also retry addons that failed on the previous run first, so a rate-limit-driven failure doesn't always strand the same addons at the tail of a fixed processing order. --- AddonCatalog.py | 10 ++ AddonCatalogCacheCreator.py | 229 +++++++++++++++++++++++++++--------- 2 files changed, 184 insertions(+), 55 deletions(-) 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..bf885ffd 100644 --- a/AddonCatalogCacheCreator.py +++ b/AddonCatalogCacheCreator.py @@ -27,7 +27,7 @@ import shutil import sys from dataclasses import is_dataclass, fields -from typing import Any, List, Optional, Dict, Tuple +from typing import Any, List, Optional, Dict, Set, Tuple import base64 import enum @@ -37,6 +37,8 @@ import os import re import requests +import time +import traceback # 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 @@ -65,6 +67,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 +131,22 @@ 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]: + """Read the previous run's clone_errors.json, if any, and return the set of addon IDs + that failed to clone/update or download, so this run retries them first. This is + deliberately based only on the immediately preceding run: an addon that succeeds this + time drops out of the priority set next time, and one that fails again stays in it.""" + 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 +154,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 +223,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 +253,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 +504,146 @@ 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 captured subprocess output, if text actually + holds any (a mock in a test, or a process that produced no output, do not), bounded 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 with a short delay in between. A timeout and a non-zero exit code + are treated identically: git's exit code doesn't reliably distinguish a transient + network blip from a permanent error, and retrying a permanent failure a couple of extra + times is cheap for an unattended job. Before every attempt, any pre-existing target_dir + is removed, since git clone refuses to run into a non-empty directory and a partial + checkout can be left behind by a killed or timed-out previous attempt. Raises + RuntimeError (with git's own stderr appended, if any was captured) if every attempt + fails; deliberately does not touch self.clone_errors, since callers use this helper for + two different targets that need different keys.""" + # 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 +744,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: From dc8815c56abf52e4941e398adc16a93427497230 Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Thu, 3 Sep 2026 13:27:44 +0200 Subject: [PATCH 2/4] TST: add unit tests --- .../app/test_addon_catalog_cache_creator.py | 208 +++++++++++++++--- AddonManagerTest/app/test_addoncatalog.py | 29 ++- 2 files changed, 207 insertions(+), 30 deletions(-) diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py index 6ea28e41..641667cf 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() @@ -291,8 +287,12 @@ def test_should_use_sparse_clone_for_a_normal_addon(self): writer = accc.CacheWriter() self.assertFalse(writer.should_use_sparse_clone("SomeNormalAddon", ace)) - @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse") - def test_create_local_copy_of_single_addon_using_sparse_clone(self, mock_create_with_sparse): + @patch( + "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse" + ) + def test_create_local_copy_of_single_addon_using_sparse_clone( + self, mock_create_with_sparse + ): """An entry that asks for a sparse cache is fetched with a sparse clone, and is marked so that clients know that only part of it is cached.""" catalog_entries = [ @@ -314,8 +314,12 @@ def test_create_local_copy_of_single_addon_using_sparse_clone(self, mock_create_ self.assertEqual(1, mock_create_with_sparse.call_count) self.assertTrue(catalog_entries[0].sparse_cache) - @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") - @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse") + @patch( + "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git" + ) + @patch( + "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse" + ) def test_create_local_copy_of_single_addon_with_impossible_sparse_clone( self, mock_create_with_sparse, mock_create_with_git ): @@ -323,7 +327,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() @@ -336,7 +344,9 @@ def test_create_local_copy_of_single_addon_with_impossible_sparse_clone( self.assertEqual(1, mock_create_with_git.call_count) self.assertFalse(catalog_entries[0].sparse_cache) - @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") + @patch( + "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git" + ) def test_create_local_copy_of_single_addon_using_git(self, mock_create_with_git): """Given a single addon, each catalog entry is fetched with git if git info is available.""" catalog_entries = [ @@ -347,7 +357,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() @@ -356,8 +370,12 @@ def test_create_local_copy_of_single_addon_using_git(self, mock_create_with_git) writer.create_local_copy_of_single_addon("TestMod", catalog_entries) self.assertEqual(mock_create_with_git.call_count, 3) - @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") - @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_zip") + @patch( + "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git" + ) + @patch( + "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_zip" + ) def test_create_local_copy_of_single_addon_using_zip( self, mock_create_with_zip, mock_create_with_git ): @@ -367,7 +385,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 +437,33 @@ 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 +494,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 +507,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 +529,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 +557,110 @@ 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): @@ -522,7 +670,9 @@ def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): writer = accc.CacheWriter() writer.sparse_clone("TestMod", "https://some.url", "main", ["package.xml"]) commands = self.issued_commands(mock_run) - self.assertEqual(["git", "fetch", "--force", "--depth=1", "origin", "main"], commands[0]) + self.assertEqual( + ["git", "fetch", "--force", "--depth=1", "origin", "main"], commands[0] + ) self.assertIn(["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], commands) self.assertEqual({}, writer.clone_errors) @@ -530,7 +680,9 @@ def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): def test_add_to_sparse_clone_checks_out_without_network_access(self, mock_run): """New sparse checkout entries are taken from the commit that is already local.""" mock_run.return_value.returncode = 0 - sparse_file = os.path.join(os.getcwd(), "TestMod", ".git", "info", "sparse-checkout") + sparse_file = os.path.join( + os.getcwd(), "TestMod", ".git", "info", "sparse-checkout" + ) self.fake_fs().create_file(sparse_file, contents="package.xml\n") writer = accc.CacheWriter() writer.add_to_sparse_clone("TestMod", ["icon.svg"]) 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 = { From dcaba5e16b5ca108593c6be7c774af861f2735a2 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:30:46 +0000 Subject: [PATCH 3/4] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../app/test_addon_catalog_cache_creator.py | 56 +++++-------------- 1 file changed, 14 insertions(+), 42 deletions(-) diff --git a/AddonManagerTest/app/test_addon_catalog_cache_creator.py b/AddonManagerTest/app/test_addon_catalog_cache_creator.py index 641667cf..f9b60b7c 100644 --- a/AddonManagerTest/app/test_addon_catalog_cache_creator.py +++ b/AddonManagerTest/app/test_addon_catalog_cache_creator.py @@ -287,12 +287,8 @@ def test_should_use_sparse_clone_for_a_normal_addon(self): writer = accc.CacheWriter() self.assertFalse(writer.should_use_sparse_clone("SomeNormalAddon", ace)) - @patch( - "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse" - ) - def test_create_local_copy_of_single_addon_using_sparse_clone( - self, mock_create_with_sparse - ): + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse") + def test_create_local_copy_of_single_addon_using_sparse_clone(self, mock_create_with_sparse): """An entry that asks for a sparse cache is fetched with a sparse clone, and is marked so that clients know that only part of it is cached.""" catalog_entries = [ @@ -314,12 +310,8 @@ def test_create_local_copy_of_single_addon_using_sparse_clone( self.assertEqual(1, mock_create_with_sparse.call_count) self.assertTrue(catalog_entries[0].sparse_cache) - @patch( - "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git" - ) - @patch( - "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse" - ) + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git_sparse") def test_create_local_copy_of_single_addon_with_impossible_sparse_clone( self, mock_create_with_sparse, mock_create_with_git ): @@ -344,9 +336,7 @@ def test_create_local_copy_of_single_addon_with_impossible_sparse_clone( self.assertEqual(1, mock_create_with_git.call_count) self.assertFalse(catalog_entries[0].sparse_cache) - @patch( - "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git" - ) + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") def test_create_local_copy_of_single_addon_using_git(self, mock_create_with_git): """Given a single addon, each catalog entry is fetched with git if git info is available.""" catalog_entries = [ @@ -370,12 +360,8 @@ def test_create_local_copy_of_single_addon_using_git(self, mock_create_with_git) writer.create_local_copy_of_single_addon("TestMod", catalog_entries) self.assertEqual(mock_create_with_git.call_count, 3) - @patch( - "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git" - ) - @patch( - "AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_zip" - ) + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_git") + @patch("AddonCatalogCacheCreator.CacheWriter.create_local_copy_of_single_addon_with_zip") def test_create_local_copy_of_single_addon_using_zip( self, mock_create_with_zip, mock_create_with_git ): @@ -457,12 +443,8 @@ def get_catalog(self): 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 - ) + 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): @@ -559,9 +541,7 @@ def test_fetch_and_reset_raises_when_reset_fails(self, mock_ref_type, mock_run): @patch("AddonCatalogCacheCreator.time.sleep") @patch("AddonCatalogCacheCreator.subprocess.run") - def test_clone_or_update_raises_after_exhausting_clone_attempts( - self, mock_run, mock_sleep - ): + 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() @@ -578,9 +558,7 @@ def test_clone_or_update_removes_leftover_directory_before_clone_attempt( ): """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") - ) + 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") @@ -641,9 +619,7 @@ def test_clone_or_update_reclone_failure_leaves_original_directory_untouched( ): """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. - ) + 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" @@ -670,9 +646,7 @@ def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): writer = accc.CacheWriter() writer.sparse_clone("TestMod", "https://some.url", "main", ["package.xml"]) commands = self.issued_commands(mock_run) - self.assertEqual( - ["git", "fetch", "--force", "--depth=1", "origin", "main"], commands[0] - ) + self.assertEqual(["git", "fetch", "--force", "--depth=1", "origin", "main"], commands[0]) self.assertIn(["git", "reset", "--hard", "FETCH_HEAD", "--quiet"], commands) self.assertEqual({}, writer.clone_errors) @@ -680,9 +654,7 @@ def test_sparse_clone_update_uses_fetch_and_reset(self, mock_run): def test_add_to_sparse_clone_checks_out_without_network_access(self, mock_run): """New sparse checkout entries are taken from the commit that is already local.""" mock_run.return_value.returncode = 0 - sparse_file = os.path.join( - os.getcwd(), "TestMod", ".git", "info", "sparse-checkout" - ) + sparse_file = os.path.join(os.getcwd(), "TestMod", ".git", "info", "sparse-checkout") self.fake_fs().create_file(sparse_file, contents="package.xml\n") writer = accc.CacheWriter() writer.add_to_sparse_clone("TestMod", ["icon.svg"]) From f17822669adfee09a49b89eb2b0c9e09e2a43329 Mon Sep 17 00:00:00 2001 From: MarcBresson Date: Thu, 3 Sep 2026 15:47:45 +0200 Subject: [PATCH 4/4] MAINT: reduce docstring's length since the code speaks for itself --- AddonCatalogCacheCreator.py | 50 +++++++++++++------------------------ 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/AddonCatalogCacheCreator.py b/AddonCatalogCacheCreator.py index bf885ffd..bde9347a 100644 --- a/AddonCatalogCacheCreator.py +++ b/AddonCatalogCacheCreator.py @@ -23,43 +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, Set, Tuple - import base64 +import datetime import enum import hashlib import io import json import os import re -import requests -import time -import traceback +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" @@ -134,10 +131,8 @@ def __init__(self): self._previously_failed_addon_ids: Set[str] = set() def _load_previously_failed_addon_ids(self) -> Set[str]: - """Read the previous run's clone_errors.json, if any, and return the set of addon IDs - that failed to clone/update or download, so this run retries them first. This is - deliberately based only on the immediately preceding run: an addon that succeeds this - time drops out of the priority set next time, and one that fails again stays in it.""" + """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() @@ -556,10 +551,9 @@ def create_local_copy_of_single_addon_with_zip( @staticmethod def _tail(text: object, limit: int = 2000) -> str: - """Return the trailing portion of some captured subprocess output, if text actually - holds any (a mock in a test, or a process that produced no output, do not), bounded 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.""" + """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() @@ -567,15 +561,7 @@ def _tail(text: object, limit: int = 2000) -> str: 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 with a short delay in between. A timeout and a non-zero exit code - are treated identically: git's exit code doesn't reliably distinguish a transient - network blip from a permanent error, and retrying a permanent failure a couple of extra - times is cheap for an unattended job. Before every attempt, any pre-existing target_dir - is removed, since git clone refuses to run into a non-empty directory and a partial - checkout can be left behind by a killed or timed-out previous attempt. Raises - RuntimeError (with git's own stderr appended, if any was captured) if every attempt - fails; deliberately does not touch self.clone_errors, since callers use this helper for - two different targets that need different keys.""" + 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"