fix(cache-downloader): Skip download if already cached

Enhances the download_dependency function to check for an existing "done" file before initiating a download, preventing unnecessary downloads of already cached dependencies. Adds unit tests to verify this behavior, ensuring that downloads are skipped when the cache is present and triggered when it is absent.

Author: mfranczel

dockerfiles/cache-downloader/cache-downloader.py

@@ -19,6 +19,12 @@ def download_dependency(
     version_path = os.path.join(BASE_PATH, release_name, f"python{python_version}")
     done_file = os.path.join(version_path, f"{python_version}-done")
 
+    if os.path.isfile(done_file):
+        print(
+            f"{datetime.datetime.now()}: {release_name} python{python_version} already cached, skipping download"
+        )
+        return
+
     # Create the version directory if it doesn't exist
     os.makedirs(version_path, exist_ok=True)
 
@@ -57,7 +63,7 @@ def download_dependency(
         f"{datetime.datetime.now()}: Done downloading {release_name} {s3_path} and extracting to {version_path}"
     )
     # Create the "done" file
-    open(done_file, "a").close()
+    Path(done_file).touch()
 
 
 def submit_downloading(

tests/unit/test_cache_downloader.py

@@ -1,7 +1,7 @@
 import importlib.util
 import os
 import shutil
-from unittest.mock import patch
+from unittest.mock import MagicMock, patch
 
 import pytest
 
@@ -21,6 +21,7 @@
 _spec.loader.exec_module(_mod)
 
 cleanup_old_versions = _mod.cleanup_old_versions
+download_dependency = _mod.download_dependency
 
 
 class TestCleanupOldVersions:
@@ -169,3 +170,44 @@ def _fail_on_v1(path, *args, **kwargs):
         assert (tmp_path / "v1").exists()
         assert (tmp_path / "v2").exists()
         assert (tmp_path / "v3").exists()
+
+
+class TestDownloadDependency:
+    def test_skips_download_when_done_file_exists(self, tmp_path):
+        """Already-cached version should not trigger a download."""
+        release = "v1.0.0"
+        py_ver = "3.11"
+        version_path = tmp_path / release / f"python{py_ver}"
+        version_path.mkdir(parents=True)
+        (version_path / f"{py_ver}-done").write_text("ok")
+
+        with patch.object(_mod, "BASE_PATH", str(tmp_path)):
+            with patch.object(_mod.subprocess, "Popen") as mock_popen:
+                download_dependency(release, py_ver, "fake-bucket")
+
+        mock_popen.assert_not_called()
+
+    def test_downloads_when_done_file_missing(self, tmp_path):
+        """Missing done file should trigger the download."""
+        release = "v1.0.0"
+        py_ver = "3.11"
+        version_path = tmp_path / release / f"python{py_ver}"
+        version_path.mkdir(parents=True)
+
+        mock_aws = MagicMock()
+        mock_aws.stdout = MagicMock()
+        mock_aws.stderr = MagicMock(read=MagicMock(return_value=b""))
+        mock_aws.wait.return_value = 0
+
+        mock_tar = MagicMock()
+        mock_tar.communicate.return_value = (b"", b"")
+        mock_tar.returncode = 0
+
+        with patch.object(_mod, "BASE_PATH", str(tmp_path)):
+            with patch.object(
+                _mod.subprocess, "Popen", side_effect=[mock_aws, mock_tar]
+            ) as mock_popen:
+                download_dependency(release, py_ver, "fake-bucket")
+
+        assert mock_popen.call_count == 2
+        assert (version_path / f"{py_ver}-done").exists()