From a877acf1d14e89509a50eaaee56ece62c4a35254 Mon Sep 17 00:00:00 2001
From: "J. Nick Koston" <nick@home-assistant.io>
Date: Sun, 26 Apr 2026 09:55:27 -0500
Subject: [PATCH] Address Copilot review on PR #16021

- download_content_many now de-duplicates by `path` so two callers
  asking for the same cache file (e.g. the same URL referenced twice
  in a config) can't race on download_content's non-atomic write. When
  duplicates are present, the first-seen URL for that path wins.
- Clamp `max_workers` to at least 1 so an invalid caller value can't
  raise ValueError out of ThreadPoolExecutor.
---
 esphome/external_files.py               | 25 ++++++++++++-----
 tests/unit_tests/test_external_files.py | 36 +++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/esphome/external_files.py b/esphome/external_files.py
index 7d27b05a56..9618757529 100644
--- a/esphome/external_files.py
+++ b/esphome/external_files.py
@@ -139,18 +139,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
diff --git a/tests/unit_tests/test_external_files.py b/tests/unit_tests/test_external_files.py
index a7d7f74895..69e1c6cdde 100644
--- a/tests/unit_tests/test_external_files.py
+++ b/tests/unit_tests/test_external_files.py
@@ -350,6 +350,42 @@ def test_download_content_many_propagates_errors(
         external_files.download_content_many(items)
 
 
+@patch("esphome.external_files.download_content")
+def test_download_content_many_dedupes_by_path(
+    mock_download: 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.call_count == 1
+    # First-seen URL wins, matching dict-insertion order.
+    args, _ = mock_download.call_args
+    assert args[0] == "https://example.com/a"
+    assert args[1] == path
+
+
+@patch("esphome.external_files.download_content")
+def test_download_content_many_clamps_invalid_max_workers(
+    mock_download: 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.call_count == 2
+
+
 @patch("esphome.external_files.download_content_many")
 def test_download_web_files_in_config_filters_and_dispatches(
     mock_many: MagicMock, setup_core: Path