diff --git a/esphome/external_files.py b/esphome/external_files.py index c7ddf0ffae..58be4a7c26 100644 --- a/esphome/external_files.py +++ b/esphome/external_files.py @@ -158,14 +158,11 @@ def has_remote_file_changed( } if etag := _read_etag(local_file_path): headers[IF_NONE_MATCH] = etag - # Retried even though a failure degrades gracefully to the - # cached copy below: allow_stale=False consumers reject an - # unverified copy, so for them a healed flake avoids a hard - # failure in download_content. Only connection-level failures - # are retryable here; HEAD never raises on an HTTP status - # (deliberately: servers that reject HEAD with 405/501 fall - # through to "changed" and the GET below handles them), so a - # 5xx lands in the != 304 branch and the GET's retry covers it. + # 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( diff --git a/esphome/net_retry.py b/esphome/net_retry.py index f783f77607..c56d45e217 100644 --- a/esphome/net_retry.py +++ b/esphome/net_retry.py @@ -12,34 +12,22 @@ import time _LOGGER = logging.getLogger(__name__) -# Transient network failures are retried with 2s/4s backoff, matching -# git.py's _NETWORK_MAX_ATTEMPTS and framework_helpers' mirror downloads. -# Worst case per fetch is NETWORK_MAX_ATTEMPTS timeouts plus 6s of sleeps; -# callers are expected to memoize failures so a flaky host pays that at -# most once per file per run. +# 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 permanent socket.gaierror hides in ``e``'s exception chain. + """Whether a hard socket.gaierror hides in ``e``'s exception chain. - EAI_AGAIN ("temporary failure in name resolution", the flaky container - resolver case) stays retryable; only hard resolution failures like - NXDOMAIN count. Everything except EAI_AGAIN is treated as permanent: - deliberately narrower than git.py, which retries NXDOMAIN too. Codes - like EAI_NONAME can occur during a brief network outage, but a 6s - backoff rarely outlives one, and permanent means callers with a cached - copy fall back to it immediately instead of sleeping first (the - offline-build case). The trade-off is that a hard resolver failure on - a first download fails without retrying, same as before retries - existed. + 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. - Only the deliberate chain is walked: ``__cause__``, ``args`` (requests - wraps MaxRetryError as ``ConnectionError(e)`` without ``from``) and - urllib3 MaxRetryError's ``reason`` attribute (NameResolutionError never - lands on the cause chain). Implicit ``__context__`` is skipped so a - resolution failure from an unrelated earlier attempt cannot reclassify - an error it did not cause. + 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 @@ -71,10 +59,8 @@ def _is_permanent_dns_failure(e: BaseException) -> bool: 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 name - resolution failures (NXDOMAIN, offline hosts; not EAI_AGAIN), other - HTTP errors, local errors, and exhausted-attempts EsphomeError - wrappers (their per-mirror retries are already spent) are permanent. + 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. @@ -99,9 +85,9 @@ def is_transient_download_error(e: Exception) -> bool: def fetch_with_retry[T](url: str, fetch: Callable[[], T], what: str = "Download") -> T: - """Run ``fetch``, retrying failures that ``is_transient_download_error`` - classifies as transient with 2s/4s backoff. Other failures and the - final attempt's failure propagate to the caller's fallback handling. + """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 diff --git a/tests/unit_tests/test_external_files.py b/tests/unit_tests/test_external_files.py index cb99593fc2..caefb7ed5c 100644 --- a/tests/unit_tests/test_external_files.py +++ b/tests/unit_tests/test_external_files.py @@ -83,10 +83,8 @@ def mock_download_content_many() -> MagicMock: @pytest.fixture def mock_retry_sleep() -> MagicMock: - """Patch the retry backoff sleep (module-global time.sleep, so the - blast radius is the whole process) for tests that inject transient - network errors; without it they would really wait 2s/4s per retry. - Retry tests assert on this mock's call args. + """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 diff --git a/tests/unit_tests/test_net_retry.py b/tests/unit_tests/test_net_retry.py index d915a0ef9b..c22bda5ee5 100644 --- a/tests/unit_tests/test_net_retry.py +++ b/tests/unit_tests/test_net_retry.py @@ -39,10 +39,8 @@ class TestIsTransientDownloadError: assert not is_transient_download_error(req.HTTPError("boom")) def test_hard_dns_failures_are_permanent(self) -> None: - """Hard name resolution failures won't heal within a retry window; - offline builds must fall back to their cache without sleeping first. - requests can surface the gaierror via the cause chain or via - urllib3's MaxRetryError.reason attribute.""" + """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") @@ -51,9 +49,8 @@ class TestIsTransientDownloadError: chained.__cause__ = gai assert not is_transient_download_error(chained) - # The real urllib3 shape, raised the way urllib3 raises it: the - # gaierror is NameResolutionError.__cause__ (set at the raise - # site), which MaxRetryError carries in its reason attribute. + # 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: @@ -66,8 +63,7 @@ class TestIsTransientDownloadError: assert is_transient_download_error(req.ConnectionError("reset by peer")) def test_temporary_dns_failure_stays_transient(self) -> None: - """EAI_AGAIN is a temporary resolver failure (flaky container DNS) - and does heal, matching git.py's policy of retrying DNS flakes.""" + """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 @@ -75,9 +71,8 @@ class TestIsTransientDownloadError: assert is_transient_download_error(chained) def test_implicit_context_does_not_reclassify(self) -> None: - """A resolution failure from an unrelated earlier attempt reaches the - final error only via implicit __context__ (raise inside an except - block); it must not turn a genuine connection reset permanent.""" + """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") @@ -133,8 +128,7 @@ class TestFetchWithRetry: def test_logs_the_upcoming_attempt_number( self, caplog: pytest.LogCaptureFixture ) -> None: - """The warning names the attempt about to run, not the one that just - failed, so a user sees attempt 2/3 and 3/3 before the final failure.""" + """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),