From 4ce4768ebd2af64ddba7741b187e51d26eaf1476 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 23 Aug 2026 15:25:39 -0500 Subject: [PATCH] [core] Retry transient network errors when downloading external files (#18538) --- esphome/external_files.py | 21 ++- esphome/framework_helpers.py | 32 +---- esphome/net_retry.py | 114 ++++++++++++++++ tests/unit_tests/test_external_files.py | 138 +++++++++++++++++++- tests/unit_tests/test_framework_helpers.py | 38 ------ tests/unit_tests/test_net_retry.py | 143 +++++++++++++++++++++ 6 files changed, 416 insertions(+), 70 deletions(-) create mode 100644 esphome/net_retry.py create mode 100644 tests/unit_tests/test_net_retry.py diff --git a/esphome/external_files.py b/esphome/external_files.py index f30d429425..58be4a7c26 100644 --- a/esphome/external_files.py +++ b/esphome/external_files.py @@ -16,6 +16,7 @@ from esphome.const import CONF_FILE, CONF_TYPE, CONF_URL, __version__ from esphome.core import CORE, EsphomeError, TimePeriodSeconds from esphome.happy_eyeballs import ensure_happy_eyeballs from esphome.helpers import write_file +from esphome.net_retry import fetch_with_retry from esphome.types import ConfigType _LOGGER = logging.getLogger(__name__) @@ -157,8 +158,17 @@ def has_remote_file_changed( } if etag := _read_etag(local_file_path): headers[IF_NONE_MATCH] = etag - response = requests.head( - url, headers=headers, timeout=timeout, allow_redirects=True + # Retried so allow_stale=False consumers don't hard-fail on a + # healed flake. Only connection-level failures retry: HEAD + # never raises on HTTP status (servers rejecting HEAD with + # 405/501 must fall through to the GET), so 5xx is handled by + # the GET's own retry. + response = fetch_with_retry( + url, + lambda: requests.head( + url, headers=headers, timeout=timeout, allow_redirects=True + ), + what="Revalidation", ) _LOGGER.debug( @@ -293,7 +303,7 @@ def download_content( _LOGGER.info("Downloading %s", url) _LOGGER.debug("Saving to %s", path) - try: + def _fetch() -> tuple[requests.Response, bytes]: req = requests.get( url, timeout=timeout, @@ -304,7 +314,10 @@ def download_content( # and mid-stream connection errors all surface here as # RequestException subclasses, so this needs the same fall-back # treatment as the request itself. - data = req.content + return req, req.content + + try: + req, data = fetch_with_retry(url, _fetch) except requests.exceptions.RequestException as e: if path.exists(): # Memoized so a flaky host warns once per run, not per consumer. diff --git a/esphome/framework_helpers.py b/esphome/framework_helpers.py index b8a43220ff..105791c518 100644 --- a/esphome/framework_helpers.py +++ b/esphome/framework_helpers.py @@ -15,6 +15,7 @@ from typing import IO, TYPE_CHECKING from esphome.happy_eyeballs import ensure_happy_eyeballs from esphome.helpers import ProgressBar, rmtree +from esphome.net_retry import NETWORK_MAX_ATTEMPTS, is_transient_download_error if TYPE_CHECKING: import requests @@ -29,8 +30,9 @@ _LOGGER = logging.getLogger(__name__) _MIRROR_ATTEMPTS = 3 # Passes over the whole mirror list when a transient network error is in -# the mix; matches git.py's _NETWORK_MAX_ATTEMPTS (3 tries, 2s/4s backoff). -_MIRROR_SWEEP_ATTEMPTS = 3 +# the mix; shares net_retry's policy (3 tries, 2s/4s backoff), which in +# turn matches git.py's _NETWORK_MAX_ATTEMPTS. +_MIRROR_SWEEP_ATTEMPTS = NETWORK_MAX_ATTEMPTS def get_project_link_flags() -> list[str]: @@ -903,30 +905,6 @@ def _spent_attempts_error(e: Exception, attempts: int) -> Exception: return err -def _is_transient_download_error(e: Exception) -> bool: - """Return True when a download failure is worth retrying. - - Connection-level failures and HTTP 429/5xx are transient. Other HTTP - errors, local errors, and exhausted-attempts EsphomeError wrappers - (their per-mirror retries are already spent) are permanent. - """ - # Imported lazily: requests is a heavy import (~85ms) and is only - # needed when actually downloading, never during config validation. - import requests - - if isinstance(e, requests.exceptions.HTTPError): - resp = e.response - return resp is not None and (resp.status_code == 429 or resp.status_code >= 500) - return isinstance( - e, - ( - requests.exceptions.ConnectionError, - requests.exceptions.Timeout, - requests.exceptions.ChunkedEncodingError, - ), - ) - - def _try_mirrors_once( urls: list[str], path_target: Path | None, @@ -1131,7 +1109,7 @@ def download_from_mirrors( # Permanent failures (404, verification mismatch) won't heal; # only retry when a transient error is in the mix (as git.py does). transient = next( - ((u, e) for u, e in sweep_failures if _is_transient_download_error(e)), + ((u, e) for u, e in sweep_failures if is_transient_download_error(e)), None, ) if transient is None: diff --git a/esphome/net_retry.py b/esphome/net_retry.py new file mode 100644 index 0000000000..f7e6e601ea --- /dev/null +++ b/esphome/net_retry.py @@ -0,0 +1,114 @@ +"""Retry policy for HTTP downloads. + +Kept import-light on purpose: this module is imported at config time, so it +must not pull in requests (a heavy import, ~85ms) at module scope. +""" + +from __future__ import annotations + +from collections.abc import Callable +import logging +import time + +_LOGGER = logging.getLogger(__name__) + +# 3 tries with 2s/4s backoff, matching git.py's _NETWORK_MAX_ATTEMPTS. +# Callers memoize failures so a flaky host pays this once per file per run. +NETWORK_MAX_ATTEMPTS = 3 + + +def _is_permanent_dns_failure(e: BaseException) -> bool: + """Whether a hard socket.gaierror hides in ``e``'s exception chain. + + EAI_AGAIN (flaky resolver) stays retryable; anything else is permanent + so offline builds fall back to their cache without sleeping first. + Narrower than git.py, which retries NXDOMAIN too. + + Walks ``__cause__``, ``args`` (requests wraps MaxRetryError without + ``from``) and MaxRetryError's ``reason``, but not implicit + ``__context__``: an unrelated earlier attempt's resolution failure + must not reclassify an error it did not cause. + """ + import socket + + seen: set[int] = set() + stack: list[BaseException] = [e] + while stack: + exc = stack.pop() + if id(exc) in seen: + continue + if ( + isinstance(exc, socket.gaierror) + and exc.errno is not None + and exc.errno != socket.EAI_AGAIN + ): + return True + seen.add(id(exc)) + stack.extend( + nxt + for nxt in ( + exc.__cause__, + getattr(exc, "reason", None), # urllib3 MaxRetryError + *exc.args, + ) + if isinstance(nxt, BaseException) + ) + return False + + +def is_transient_download_error(e: Exception) -> bool: + """Return True when a download failure is worth retrying. + + Connection-level failures and HTTP 429/5xx are transient; hard DNS + failures, other HTTP errors, and local errors are permanent. + """ + # Imported lazily: requests is a heavy import (~85ms) and is only + # needed when actually downloading, never during config validation. + import requests + + if isinstance(e, requests.exceptions.HTTPError): + resp = e.response + return resp is not None and (resp.status_code == 429 or resp.status_code >= 500) + if isinstance(e, requests.exceptions.ConnectionError) and _is_permanent_dns_failure( + e + ): + return False + # SSLError (a ConnectionError subclass) stays transient on purpose: it + # also covers mid-handshake connection drops, not just bad certificates. + return isinstance( + e, + ( + requests.exceptions.ConnectionError, + requests.exceptions.Timeout, + requests.exceptions.ChunkedEncodingError, + requests.exceptions.ContentDecodingError, + ), + ) + + +def fetch_with_retry[T](url: str, fetch: Callable[[], T], what: str = "Download") -> T: + """Run ``fetch``, retrying transient failures with 2s/4s backoff. + + Permanent failures and the final attempt propagate to the caller; + ``what`` names the operation in the retry warning. + """ + import requests + + for attempt in range(1, NETWORK_MAX_ATTEMPTS): + try: + return fetch() + except requests.exceptions.RequestException as e: + if not is_transient_download_error(e): + raise + delay = 2**attempt + _LOGGER.warning( + "%s of %s failed: %s. Retrying in %d seconds... (attempt %d/%d)", + what, + url, + e, + delay, + attempt + 1, + NETWORK_MAX_ATTEMPTS, + ) + time.sleep(delay) + return fetch() diff --git a/tests/unit_tests/test_external_files.py b/tests/unit_tests/test_external_files.py index 4e993ff4f3..caefb7ed5c 100644 --- a/tests/unit_tests/test_external_files.py +++ b/tests/unit_tests/test_external_files.py @@ -4,7 +4,7 @@ import os from pathlib import Path import time from typing import Any -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch import pytest import requests @@ -81,6 +81,15 @@ def mock_download_content_many() -> MagicMock: yield m +@pytest.fixture +def mock_retry_sleep() -> MagicMock: + """Patch the retry backoff sleep (process-wide; net_retry.time is the + global module) so transient-error tests don't really wait 2s/4s. + """ + with patch("esphome.net_retry.time.sleep") as m: + yield m + + def test_compute_local_file_dir(setup_core: Path) -> None: """Test compute_local_file_dir creates and returns correct path.""" domain = "font" @@ -495,6 +504,7 @@ class _BodyReadErrorResponse: def test_download_content_with_body_read_error_uses_cache( mock_has_remote_file_changed: MagicMock, mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, setup_core: Path, ) -> None: """Body-read errors (chunked-decode/gzip-decode/mid-stream connection @@ -519,6 +529,7 @@ def test_download_content_with_body_read_error_uses_cache( def test_download_content_with_body_read_error_no_cache_fails( mock_has_remote_file_changed: MagicMock, mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, setup_core: Path, ) -> None: """A body-read failure with no cache available must surface as a @@ -535,6 +546,131 @@ def test_download_content_with_body_read_error_no_cache_fails( external_files.download_content("https://example.com/file.txt", test_file) +def test_download_content_retries_transient_error_then_succeeds( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """Transient failures (connection reset, timeout) are retried with 2s/4s + backoff before giving up; a late success downloads normally.""" + test_file = setup_core / "downloads" / "file.txt" + mock_has_remote_file_changed.return_value = True + + ok = MagicMock() + ok.content = b"downloaded" + ok.headers = {} + mock_requests_get.side_effect = [ + requests.exceptions.ConnectionError("reset by peer"), + requests.exceptions.Timeout("timed out"), + ok, + ] + + result = external_files.download_content("https://example.com/file.txt", test_file) + + assert result == b"downloaded" + assert test_file.read_bytes() == b"downloaded" + assert mock_retry_sleep.call_args_list == [call(2), call(4)] + + +def test_download_content_transient_error_exhausts_attempts( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """A persistent transient failure gives up after three attempts and then + follows the normal no-cache error path.""" + test_file = setup_core / "nonexistent.txt" + mock_has_remote_file_changed.return_value = True + mock_requests_get.side_effect = requests.exceptions.ConnectionError("reset by peer") + + with pytest.raises(Invalid, match="Could not download from.*reset by peer"): + external_files.download_content("https://example.com/file.txt", test_file) + + assert mock_retry_sleep.call_args_list == [call(2), call(4)] + + +def test_download_content_non_transient_error_not_retried( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """Permanent failures like a 404 fail on the first attempt.""" + test_file = setup_core / "nonexistent.txt" + mock_has_remote_file_changed.return_value = True + + response = MagicMock() + response.status_code = 404 + mock_requests_get.side_effect = requests.exceptions.HTTPError( + "404 Client Error", response=response + ) + + with pytest.raises(Invalid, match="Could not download from.*404"): + external_files.download_content("https://example.com/file.txt", test_file) + + assert mock_requests_get.call_count == 1 + mock_retry_sleep.assert_not_called() + + +def test_download_content_retries_body_read_error( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, +) -> None: + """Mid-stream failures surfacing from `.content` are retried too.""" + test_file = setup_core / "downloads" / "file.txt" + mock_has_remote_file_changed.return_value = True + + ok = MagicMock() + ok.content = b"downloaded" + ok.headers = {} + mock_requests_get.side_effect = [ + _BodyReadErrorResponse( + requests.exceptions.ChunkedEncodingError("body truncated") + ), + ok, + ] + + result = external_files.download_content("https://example.com/file.txt", test_file) + + assert result == b"downloaded" + assert mock_requests_get.call_count == 2 + assert mock_retry_sleep.call_args_list == [call(2)] + + +def test_has_remote_file_changed_retries_transient_error( + mock_requests_head: MagicMock, + mock_retry_sleep: MagicMock, + setup_core: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """A HEAD revalidation that fails transiently then returns 304 does not + mark the cached copy stale, and the retry warning names the operation.""" + test_file = setup_core / "cached.txt" + test_file.write_bytes(b"cached content") + + ok = MagicMock() + ok.status_code = 304 + ok.headers = {} + mock_requests_head.side_effect = [ + requests.exceptions.ConnectionError("reset by peer"), + ok, + ] + + changed = external_files.has_remote_file_changed( + "https://example.com/file.txt", test_file + ) + + assert changed is False + assert test_file not in external_files._run_data().stale_paths + assert mock_requests_head.call_count == 2 + assert mock_retry_sleep.call_args_list == [call(2)] + assert "Revalidation of" in caplog.text + + def test_download_content_skip_external_update_uses_cache( mock_has_remote_file_changed: MagicMock, mock_requests_get: MagicMock, diff --git a/tests/unit_tests/test_framework_helpers.py b/tests/unit_tests/test_framework_helpers.py index 2022c15bfe..500705ef67 100644 --- a/tests/unit_tests/test_framework_helpers.py +++ b/tests/unit_tests/test_framework_helpers.py @@ -23,7 +23,6 @@ from esphome.core import EsphomeError from esphome.framework_helpers import ( _7z_extract_all, _detect_archive_root, - _is_transient_download_error, _rename_with_retry, _tar_extract_all, _zip_extract_all, @@ -1594,43 +1593,6 @@ class TestDownloadFromMirrors: mock_sleep.assert_not_called() -def _http_error(status: int) -> req.HTTPError: - """An HTTPError carrying a response with the given status, as raised by - ``raise_for_status`` on a real response.""" - resp = MagicMock() - resp.status_code = status - return req.HTTPError(str(status), response=resp) - - -class TestIsTransientDownloadError: - def test_connection_errors_are_transient(self) -> None: - assert _is_transient_download_error(req.ConnectionError("reset")) - assert _is_transient_download_error(req.Timeout("timed out")) - assert _is_transient_download_error( - req.exceptions.ChunkedEncodingError("dropped") - ) - - def test_http_statuses(self) -> None: - assert not _is_transient_download_error(_http_error(404)) - assert not _is_transient_download_error(_http_error(403)) - assert _is_transient_download_error(_http_error(429)) - assert _is_transient_download_error(_http_error(503)) - - def test_http_error_without_response_is_permanent(self) -> None: - assert not _is_transient_download_error(req.HTTPError("boom")) - - def test_exhausted_resume_attempts_are_permanent(self) -> None: - """download_with_resume already spent its own resume attempts; its - EsphomeError wrapper is not retried again at the sweep level.""" - wrapped = EsphomeError("Failed to download after 3 attempts") - wrapped.__cause__ = req.ConnectionError("down") - assert not _is_transient_download_error(wrapped) - - def test_unrelated_errors_are_permanent(self) -> None: - assert not _is_transient_download_error(OSError("disk full")) - assert not _is_transient_download_error(EsphomeError("size mismatch")) - - def test_importing_framework_helpers_does_not_import_requests() -> None: """Importing framework_helpers must not drag in requests. diff --git a/tests/unit_tests/test_net_retry.py b/tests/unit_tests/test_net_retry.py new file mode 100644 index 0000000000..c22bda5ee5 --- /dev/null +++ b/tests/unit_tests/test_net_retry.py @@ -0,0 +1,143 @@ +"""Tests for esphome.net_retry.""" + +import socket +from unittest.mock import MagicMock, call, patch + +import pytest +import requests as req + +from esphome.core import EsphomeError +from esphome.net_retry import fetch_with_retry, is_transient_download_error + + +def _http_error(status: int) -> req.HTTPError: + """An HTTPError carrying a response with the given status, as raised by + ``raise_for_status`` on a real response.""" + resp = MagicMock() + resp.status_code = status + return req.HTTPError(str(status), response=resp) + + +class TestIsTransientDownloadError: + def test_connection_errors_are_transient(self) -> None: + assert is_transient_download_error(req.ConnectionError("reset")) + assert is_transient_download_error(req.Timeout("timed out")) + assert is_transient_download_error( + req.exceptions.ChunkedEncodingError("dropped") + ) + assert is_transient_download_error( + req.exceptions.ContentDecodingError("gzip stream truncated") + ) + + def test_http_statuses(self) -> None: + assert not is_transient_download_error(_http_error(404)) + assert not is_transient_download_error(_http_error(403)) + assert is_transient_download_error(_http_error(429)) + assert is_transient_download_error(_http_error(503)) + + def test_http_error_without_response_is_permanent(self) -> None: + assert not is_transient_download_error(req.HTTPError("boom")) + + def test_hard_dns_failures_are_permanent(self) -> None: + """Hard resolution failures are permanent via both the cause chain + and MaxRetryError.reason.""" + from urllib3.exceptions import MaxRetryError, NameResolutionError + + gai = socket.gaierror(socket.EAI_NONAME, "nodename nor servname provided") + + chained = req.ConnectionError("resolution failed") + chained.__cause__ = gai + assert not is_transient_download_error(chained) + + # The real urllib3 shape: gaierror on NameResolutionError.__cause__, + # carried by MaxRetryError.reason. + try: + raise NameResolutionError("example.invalid", None, gai) from gai + except NameResolutionError as nre: + wrapped = req.ConnectionError( + MaxRetryError(None, "http://example.invalid/", reason=nre) + ) + assert not is_transient_download_error(wrapped) + + # A garden-variety connection reset stays transient. + assert is_transient_download_error(req.ConnectionError("reset by peer")) + + def test_temporary_dns_failure_stays_transient(self) -> None: + """EAI_AGAIN (flaky resolver) stays retryable.""" + gai = socket.gaierror(socket.EAI_AGAIN, "temporary failure in name resolution") + chained = req.ConnectionError("resolution failed") + chained.__cause__ = gai + + assert is_transient_download_error(chained) + + def test_implicit_context_does_not_reclassify(self) -> None: + """A gaierror riding along as implicit __context__ must not turn a + genuine connection reset permanent.""" + try: + try: + raise socket.gaierror(socket.EAI_NONAME, "first attempt") + except socket.gaierror: + raise req.ConnectionError("reset by peer") from None + except req.ConnectionError as reset: + assert reset.__context__ is not None + assert is_transient_download_error(reset) + + def test_gaierror_without_errno_stays_transient(self) -> None: + """A gaierror carrying no EAI code cannot prove a hard failure.""" + chained = req.ConnectionError("resolution failed") + chained.__cause__ = socket.gaierror("no errno") + + assert is_transient_download_error(chained) + + def test_mixed_chain_hard_failure_wins(self) -> None: + """EAI_AGAIN in the chain does not mask a hard failure elsewhere.""" + again = socket.gaierror(socket.EAI_AGAIN, "temporary failure") + hard = socket.gaierror(socket.EAI_NONAME, "unknown host") + + outer = req.ConnectionError(hard) + outer.__cause__ = again + assert not is_transient_download_error(outer) + + outer = req.ConnectionError(again) + outer.__cause__ = hard + assert not is_transient_download_error(outer) + + def test_dns_walk_survives_exception_cycles(self) -> None: + """A cyclic cause chain must terminate (and stay transient when no + resolution failure is present).""" + outer = req.ConnectionError("a") + inner = ValueError("b") + outer.__cause__ = inner + inner.__cause__ = outer + + assert is_transient_download_error(outer) + + def test_exhausted_resume_attempts_are_permanent(self) -> None: + """download_with_resume already spent its own resume attempts; its + EsphomeError wrapper is not retried again at the sweep level.""" + wrapped = EsphomeError("Failed to download after 3 attempts") + wrapped.__cause__ = req.ConnectionError("down") + assert not is_transient_download_error(wrapped) + + def test_unrelated_errors_are_permanent(self) -> None: + assert not is_transient_download_error(OSError("disk full")) + assert not is_transient_download_error(EsphomeError("size mismatch")) + + +class TestFetchWithRetry: + def test_logs_the_upcoming_attempt_number( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The warning names the attempt about to run, not the failed one.""" + with ( + patch("esphome.net_retry.time.sleep") as mock_sleep, + pytest.raises(req.ConnectionError), + ): + fetch_with_retry( + "https://example.com/f", + lambda: (_ for _ in ()).throw(req.ConnectionError("reset")), + ) + + assert mock_sleep.call_args_list == [call(2), call(4)] + assert "(attempt 2/3)" in caplog.text + assert "(attempt 3/3)" in caplog.text