From 9f058ac1a6bd0cc5e15e15ebb4e23b45574b3ece Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 26 Apr 2026 09:26:20 -0500 Subject: [PATCH] [core] Use ETag in external_files cache to fix re-downloads from raw.githubusercontent.com raw.githubusercontent.com ignores If-Modified-Since (always returns 200), but honors If-None-Match with ETag (returns 304). This caused every esphome compile/config run to re-download every cached external file (audio_file, micro_wake_word, image, font, bme68x_bsec2, etc.) sourced from a /raw/ URL. - Send If-None-Match with the cached ETag when present - Persist the ETag from each download in a hidden sidecar file (.{name}.etag) and refresh it when a 304 carries a new ETag - Replace path.write_bytes() with helpers.write_file() so downloads are written atomically and can no longer leave partially-written cache files behind on crash --- esphome/external_files.py | 43 ++++++++- tests/unit_tests/test_external_files.py | 122 ++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 4 deletions(-) diff --git a/esphome/external_files.py b/esphome/external_files.py index b6f6149ebb..2dd1cf9af6 100644 --- a/esphome/external_files.py +++ b/esphome/external_files.py @@ -1,5 +1,6 @@ from __future__ import annotations +import contextlib from datetime import UTC, datetime import logging from pathlib import Path @@ -8,7 +9,8 @@ import requests import esphome.config_validation as cv from esphome.const import __version__ -from esphome.core import CORE, TimePeriodSeconds +from esphome.core import CORE, EsphomeError, TimePeriodSeconds +from esphome.helpers import write_file _LOGGER = logging.getLogger(__name__) CODEOWNERS = ["@landonr"] @@ -16,12 +18,38 @@ CODEOWNERS = ["@landonr"] NETWORK_TIMEOUT = 30 IF_MODIFIED_SINCE = "If-Modified-Since" +IF_NONE_MATCH = "If-None-Match" +ETAG = "ETag" CACHE_CONTROL = "Cache-Control" CACHE_CONTROL_MAX_AGE = "max-age=" CONTENT_DISPOSITION = "content-disposition" TEMP_DIR = "temp" +def _etag_sidecar_path(local_file_path: Path) -> Path: + return local_file_path.parent / f".{local_file_path.name}.etag" + + +def _read_etag(local_file_path: Path) -> str | None: + try: + etag = _etag_sidecar_path(local_file_path).read_text().strip() + except OSError: + return None + return etag or None + + +def _write_etag(local_file_path: Path, etag: str | None) -> None: + etag_path = _etag_sidecar_path(local_file_path) + if not etag: + with contextlib.suppress(FileNotFoundError): + etag_path.unlink() + return + try: + write_file(etag_path, etag) + except EsphomeError as e: + _LOGGER.debug("Could not save ETag for %s: %s", local_file_path, e) + + def has_remote_file_changed(url: str, local_file_path: Path) -> bool: if local_file_path.exists(): _LOGGER.debug("has_remote_file_changed: File exists at %s", local_file_path) @@ -35,14 +63,18 @@ def has_remote_file_changed(url: str, local_file_path: Path) -> bool: IF_MODIFIED_SINCE: local_modification_time_str, CACHE_CONTROL: CACHE_CONTROL_MAX_AGE + "3600", } + etag = _read_etag(local_file_path) + if etag: + headers[IF_NONE_MATCH] = etag response = requests.head( url, headers=headers, timeout=NETWORK_TIMEOUT, allow_redirects=True ) _LOGGER.debug( - "has_remote_file_changed: File %s, Local modified %s, response code %d", + "has_remote_file_changed: File %s, Local modified %s, ETag %s, response code %d", local_file_path, local_modification_time_str, + etag or "", response.status_code, ) @@ -51,6 +83,9 @@ def has_remote_file_changed(url: str, local_file_path: Path) -> bool: "has_remote_file_changed: File not modified since %s", local_modification_time_str, ) + new_etag = response.headers.get(ETAG) + if new_etag and new_etag != etag: + _write_etag(local_file_path, new_etag) return False _LOGGER.debug("has_remote_file_changed: File modified") return True @@ -112,7 +147,7 @@ def download_content(url: str, path: Path, timeout: int = NETWORK_TIMEOUT) -> by return path.read_bytes() raise cv.Invalid(f"Could not download from {url}: {e}") from e - path.parent.mkdir(parents=True, exist_ok=True) data = req.content - path.write_bytes(data) + write_file(path, data) + _write_etag(path, req.headers.get(ETAG)) return data diff --git a/tests/unit_tests/test_external_files.py b/tests/unit_tests/test_external_files.py index 4b0826db04..ade1e66c53 100644 --- a/tests/unit_tests/test_external_files.py +++ b/tests/unit_tests/test_external_files.py @@ -98,6 +98,7 @@ def test_has_remote_file_changed_not_modified( mock_response = MagicMock() mock_response.status_code = 304 + mock_response.headers = {} mock_head.return_value = mock_response url = "https://example.com/file.txt" @@ -122,6 +123,7 @@ def test_has_remote_file_changed_modified( mock_response = MagicMock() mock_response.status_code = 200 + mock_response.headers = {} mock_head.return_value = mock_response url = "https://example.com/file.txt" @@ -166,6 +168,7 @@ def test_has_remote_file_changed_timeout( mock_response = MagicMock() mock_response.status_code = 304 + mock_response.headers = {} mock_head.return_value = mock_response url = "https://example.com/file.txt" @@ -175,6 +178,68 @@ def test_has_remote_file_changed_timeout( assert call_args[1]["timeout"] == external_files.NETWORK_TIMEOUT +@patch("esphome.external_files.requests.head") +def test_has_remote_file_changed_uses_etag( + mock_head: MagicMock, setup_core: Path +) -> None: + """Test has_remote_file_changed sends If-None-Match when ETag is cached.""" + test_file = setup_core / "cached.txt" + test_file.write_text("cached content") + external_files._etag_sidecar_path(test_file).write_text('"abc123"') + + mock_response = MagicMock() + mock_response.status_code = 304 + mock_response.headers = {} + mock_head.return_value = mock_response + + url = "https://example.com/file.txt" + result = external_files.has_remote_file_changed(url, test_file) + + assert result is False + headers = mock_head.call_args[1]["headers"] + assert headers[external_files.IF_NONE_MATCH] == '"abc123"' + + +@patch("esphome.external_files.requests.head") +def test_has_remote_file_changed_no_etag_no_if_none_match( + mock_head: MagicMock, setup_core: Path +) -> None: + """Test has_remote_file_changed omits If-None-Match when no ETag is cached.""" + test_file = setup_core / "cached.txt" + test_file.write_text("cached content") + + mock_response = MagicMock() + mock_response.status_code = 304 + mock_response.headers = {} + mock_head.return_value = mock_response + + url = "https://example.com/file.txt" + external_files.has_remote_file_changed(url, test_file) + + headers = mock_head.call_args[1]["headers"] + assert external_files.IF_NONE_MATCH not in headers + + +@patch("esphome.external_files.requests.head") +def test_has_remote_file_changed_refreshes_etag_on_304( + mock_head: MagicMock, setup_core: Path +) -> None: + """Test has_remote_file_changed updates the cached ETag when the 304 sends a new one.""" + test_file = setup_core / "cached.txt" + test_file.write_text("cached content") + external_files._etag_sidecar_path(test_file).write_text('"old"') + + mock_response = MagicMock() + mock_response.status_code = 304 + mock_response.headers = {external_files.ETAG: '"new"'} + mock_head.return_value = mock_response + + url = "https://example.com/file.txt" + external_files.has_remote_file_changed(url, test_file) + + assert external_files._etag_sidecar_path(test_file).read_text() == '"new"' + + def test_compute_local_file_dir_creates_parent_dirs(setup_core: Path) -> None: """Test compute_local_file_dir creates parent directories.""" domain = "level1/level2/level3/level4" @@ -273,6 +338,7 @@ def test_download_content_skip_external_update_downloads_when_missing( mock_has_changed.return_value = True mock_response = MagicMock() mock_response.content = new_content + mock_response.headers = {} mock_response.raise_for_status = MagicMock() mock_get.return_value = mock_response @@ -282,3 +348,59 @@ def test_download_content_skip_external_update_downloads_when_missing( assert result == new_content assert test_file.read_bytes() == new_content + + +@patch("esphome.external_files.requests.get") +@patch("esphome.external_files.has_remote_file_changed") +def test_download_content_saves_etag( + mock_has_changed: MagicMock, + mock_get: MagicMock, + setup_core: Path, +) -> None: + """Test download_content writes the ETag sidecar after a successful download.""" + test_file = setup_core / "fresh.txt" + new_content = b"fresh content" + + mock_has_changed.return_value = True + mock_response = MagicMock() + mock_response.content = new_content + mock_response.headers = {external_files.ETAG: '"deadbeef"'} + mock_response.raise_for_status = MagicMock() + mock_get.return_value = mock_response + + url = "https://example.com/file.txt" + external_files.download_content(url, test_file) + + assert external_files._etag_sidecar_path(test_file).read_text() == '"deadbeef"' + + +@patch("esphome.external_files.requests.get") +@patch("esphome.external_files.has_remote_file_changed") +def test_download_content_atomic_write_no_partial_on_failure( + mock_has_changed: MagicMock, + mock_get: MagicMock, + setup_core: Path, +) -> None: + """Test download_content does not corrupt the existing file if the write step fails.""" + test_file = setup_core / "cached.txt" + original_content = b"original content" + test_file.write_bytes(original_content) + + mock_has_changed.return_value = True + mock_response = MagicMock() + # Accessing .content raises, simulating a streaming/decode failure + type(mock_response).content = property( + lambda self: (_ for _ in ()).throw(OSError("disk full")) + ) + mock_response.raise_for_status = MagicMock() + mock_get.return_value = mock_response + + url = "https://example.com/file.txt" + with pytest.raises(OSError, match="disk full"): + external_files.download_content(url, test_file) + + # Original file is untouched (atomic rename never happened) + assert test_file.read_bytes() == original_content + # No leftover temp files from tempfile.NamedTemporaryFile + leftover_tmps = list(setup_core.glob("tmp*")) + assert leftover_tmps == []