Merge remote-tracking branch 'origin/external-files-parallel' into integration

# Conflicts:
#	tests/unit_tests/test_external_files.py

esphome/external_files.py

@@ -210,18 +210,31 @@ def download_content_many(
     Wall time drops from `sum(latency)` to roughly `max(latency)` for cached
     files where the HEAD round-trip dominates. The first exception raised by
     any worker is propagated; remaining workers complete before this returns.
+
+    Items are de-duplicated by `path` -- two callers asking for the same
+    cache file (e.g. the same URL referenced twice in a config) would
+    otherwise race on `download_content`'s non-atomic write.
     """
-    items = list(items)
-    if not items:
+    seen: dict[Path, str] = {}
+    for url, path in items:
+        if path in seen:
+            continue
+        seen[path] = url
+    if not seen:
         return
-    if len(items) == 1:
-        url, path = items[0]
+    if len(seen) == 1:
+        path, url = next(iter(seen.items()))
         download_content(url, path, timeout)
         return
-    workers = min(max_workers, len(items))
+    workers = max(1, min(max_workers, len(seen)))
     with ThreadPoolExecutor(max_workers=workers) as ex:
         # list() forces iteration so exceptions surface here, not silently.
-        list(ex.map(lambda item: download_content(item[0], item[1], timeout), items))
+        list(
+            ex.map(
+                lambda item: download_content(item[1], item[0], timeout),
+                seen.items(),
+            )
+        )
 
 
 # String constant rather than `from .const import TYPE_WEB` because each

tests/unit_tests/test_external_files.py

@@ -24,6 +24,24 @@ def _seed_etag(cache_file: Path, etag: str) -> Path:
     return sidecar
 
 
+@pytest.fixture
+def mock_download_content() -> MagicMock:
+    """Patch `external_files.download_content` for tests that exercise the
+    parallel batch helper without doing real I/O.
+    """
+    with patch("esphome.external_files.download_content") as m:
+        yield m
+
+
+@pytest.fixture
+def mock_download_content_many() -> MagicMock:
+    """Patch `external_files.download_content_many` for tests that exercise
+    the URL-collection helper without dispatching to the thread pool.
+    """
+    with patch("esphome.external_files.download_content_many") 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"
@@ -475,30 +493,27 @@ def test_download_content_atomic_write_no_partial_on_failure(
     assert leftover_tmps == []
 
 
-@patch("esphome.external_files.download_content")
 def test_download_content_many_empty_is_noop(
-    mock_download: MagicMock, setup_core: Path
+    mock_download_content: MagicMock, setup_core: Path
 ) -> None:
     """Empty input shouldn't spin up a thread pool or call download_content."""
     external_files.download_content_many([])
-    mock_download.assert_not_called()
+    mock_download_content.assert_not_called()
 
 
-@patch("esphome.external_files.download_content")
 def test_download_content_many_single_item_avoids_pool(
-    mock_download: MagicMock, setup_core: Path
+    mock_download_content: MagicMock, setup_core: Path
 ) -> None:
     """A single item should be downloaded inline (no thread pool overhead)."""
     item = ("https://example.com/file.txt", setup_core / "f.txt")
     external_files.download_content_many([item])
-    mock_download.assert_called_once_with(
+    mock_download_content.assert_called_once_with(
         item[0], item[1], external_files.NETWORK_TIMEOUT
     )
 
 
-@patch("esphome.external_files.download_content")
 def test_download_content_many_runs_in_parallel(
-    mock_download: MagicMock, setup_core: Path
+    mock_download_content: MagicMock, setup_core: Path
 ) -> None:
     """Multiple items should run concurrently — total wall time ≈ max latency."""
     import threading
@@ -511,19 +526,18 @@ def test_download_content_many_runs_in_parallel(
         barrier.wait(timeout=2.0)
         return b""
 
-    mock_download.side_effect = slow_download
+    mock_download_content.side_effect = slow_download
     items = [
         ("https://example.com/a", setup_core / "a"),
         ("https://example.com/b", setup_core / "b"),
         ("https://example.com/c", setup_core / "c"),
     ]
     external_files.download_content_many(items, max_workers=4)
-    assert mock_download.call_count == 3
+    assert mock_download_content.call_count == 3
 
 
-@patch("esphome.external_files.download_content")
 def test_download_content_many_propagates_errors(
-    mock_download: MagicMock, setup_core: Path
+    mock_download_content: MagicMock, setup_core: Path
 ) -> None:
     """An exception from any worker must propagate out of download_content_many."""
 
@@ -532,7 +546,7 @@ def test_download_content_many_propagates_errors(
             raise Invalid(f"could not download {url}")
         return b""
 
-    mock_download.side_effect = fake_download
+    mock_download_content.side_effect = fake_download
     items = [
         ("https://example.com/ok", setup_core / "ok"),
         ("https://example.com/bad", setup_core / "bad"),
@@ -541,9 +555,42 @@ def test_download_content_many_propagates_errors(
         external_files.download_content_many(items)
 
 
-@patch("esphome.external_files.download_content_many")
+def test_download_content_many_dedupes_by_path(
+    mock_download_content: MagicMock, setup_core: Path
+) -> None:
+    """Two items pointing at the same cache path must collapse to one
+    download -- otherwise concurrent writes race on the same file.
+    """
+    path = setup_core / "shared"
+    items = [
+        ("https://example.com/a", path),
+        ("https://example.com/b", path),
+        ("https://example.com/a", path),
+    ]
+    external_files.download_content_many(items)
+    assert mock_download_content.call_count == 1
+    # First-seen URL wins, matching dict-insertion order.
+    args, _ = mock_download_content.call_args
+    assert args[0] == "https://example.com/a"
+    assert args[1] == path
+
+
+def test_download_content_many_clamps_invalid_max_workers(
+    mock_download_content: MagicMock, setup_core: Path
+) -> None:
+    """`max_workers <= 0` must not raise from ThreadPoolExecutor; it should
+    be clamped up to at least 1 worker.
+    """
+    items = [
+        ("https://example.com/a", setup_core / "a"),
+        ("https://example.com/b", setup_core / "b"),
+    ]
+    external_files.download_content_many(items, max_workers=0)
+    assert mock_download_content.call_count == 2
+
+
 def test_download_web_files_in_config_filters_and_dispatches(
-    mock_many: MagicMock, setup_core: Path
+    mock_download_content_many: MagicMock, setup_core: Path
 ) -> None:
     """Only `file.type == "web"` entries should be forwarded to
     download_content_many, and the unmodified config should be returned so
@@ -562,16 +609,15 @@ def test_download_web_files_in_config_filters_and_dispatches(
     result = external_files.download_web_files_in_config(config, path_for)
 
     assert result is config
-    mock_many.assert_called_once()
-    assert list(mock_many.call_args[0][0]) == [
+    mock_download_content_many.assert_called_once()
+    assert list(mock_download_content_many.call_args[0][0]) == [
         ("https://example.com/a", setup_core / "a"),
         ("https://example.com/c", setup_core / "c"),
     ]
 
 
-@patch("esphome.external_files.download_content_many")
 def test_download_web_files_in_config_no_web_entries(
-    mock_many: MagicMock, setup_core: Path
+    mock_download_content_many: MagicMock, setup_core: Path
 ) -> None:
     """A config with no web entries should still call through to
     download_content_many (which is itself a no-op for empty input) so the
@@ -579,5 +625,5 @@ def test_download_web_files_in_config_no_web_entries(
     """
     config = [{"file": {"type": "local", "path": "/tmp/a"}}]
     external_files.download_web_files_in_config(config, lambda _: setup_core / "x")
-    mock_many.assert_called_once()
-    assert list(mock_many.call_args[0][0]) == []
+    mock_download_content_many.assert_called_once()
+    assert list(mock_download_content_many.call_args[0][0]) == []