fix(toolkit): fix race conditions in download_utils (#258)

* fix(toolkit): fix race conditions in download_utils

* cleanup temp_dir

* add suffix

projects/fal/src/fal/toolkit/utils/download_utils.py

@@ -1,11 +1,12 @@
 from __future__ import annotations
 
 import hashlib
+import os
 import shutil
 import subprocess
 import sys
 from pathlib import Path, PurePath
-from tempfile import TemporaryDirectory
+from tempfile import NamedTemporaryFile, TemporaryDirectory
 from urllib.parse import urlparse
 from urllib.request import Request, urlopen
 
@@ -215,10 +216,14 @@ def _download_file_python(
     Returns:
         The path where the downloaded file has been saved.
     """
-    import shutil
-    import tempfile
-
-    with tempfile.NamedTemporaryFile(delete=False) as temp_file:
+    basename = os.path.basename(target_path)
+    # NOTE: using the same directory to avoid potential copies across temp fs and target
+    # fs, and also to be able to atomically rename a downloaded file into place.
+    with NamedTemporaryFile(
+        delete=False,
+        dir=os.path.dirname(target_path),
+        prefix=f"{basename}.tmp",
+    ) as temp_file:
         try:
             file_path = temp_file.name
 
@@ -232,13 +237,14 @@ def _download_file_python(
 
                 print(progress_msg, end="\r\n")
 
-            # Move the file when the file is downloaded completely. Since the
-            # file used is temporary, in a case of an interruption, the downloaded
-            # content will be lost. So, it is safe to redownload the file in such cases.
-            shutil.move(file_path, target_path)
+            # NOTE: Atomically renaming the file into place when the file is downloaded
+            # completely.
+            #
+            # Since the file used is temporary, in a case of an interruption, the
+            # downloaded content will be lost. So, it is safe to redownload the file in
+            # such cases.
+            os.rename(file_path, target_path)
 
-        except Exception as error:
-            raise error
         finally:
             Path(temp_file.name).unlink(missing_ok=True)
 
@@ -403,35 +409,38 @@ def clone_repository(
             print(f"Removing the existing repository: {local_repo_path} ")
             shutil.rmtree(local_repo_path)
 
-    # Temporary directory to download the repo into.
-    temp_dir = TemporaryDirectory()
-    temp_dir_path = temp_dir.name
-
-    try:
-        print(f"Cloning the repository '{https_url}' .")
-
-        # Clone with disabling the logs and advices for detached HEAD state.
-        clone_command = [
-            "git",
-            "clone",
-            "--recursive",
-            https_url,
-            temp_dir_path,
-        ]
-        subprocess.check_call(clone_command)
-
-        if commit_hash:
-            checkout_command = ["git", "checkout", commit_hash]
-            subprocess.check_call(checkout_command, cwd=temp_dir_path)
-
-        # Move the repository when the clone and checkout is finished.
-        shutil.move(temp_dir_path, local_repo_path)
-
-    except Exception as error:
-        print(f"{error}\nFailed to clone repository '{https_url}' .")
-        temp_dir.cleanup()
+    # NOTE: using the target_dir to be able to avoid potential copies across temp fs
+    # and target fs, and also to be able to atomically rename repo_name dir into place
+    # when we are done setting it up.
+    os.makedirs(target_dir, exist_ok=True)  # type: ignore[arg-type]
+    with TemporaryDirectory(
+        dir=target_dir,
+        suffix=f"{local_repo_path.name}.tmp",
+    ) as temp_dir:
+        try:
+            print(f"Cloning the repository '{https_url}' .")
+
+            # Clone with disabling the logs and advices for detached HEAD state.
+            clone_command = [
+                "git",
+                "clone",
+                "--recursive",
+                https_url,
+                temp_dir,
+            ]
+            subprocess.check_call(clone_command)
+
+            if commit_hash:
+                checkout_command = ["git", "checkout", commit_hash]
+                subprocess.check_call(checkout_command, cwd=temp_dir)
+
+            # NOTE: Atomically renaming the repository directory into place when the
+            # clone and checkout are done.
+            os.rename(temp_dir, local_repo_path)
 
-        raise error
+        except Exception as error:
+            print(f"{error}\nFailed to clone repository '{https_url}' .")
+            raise error
 
     if include_to_path:
         __add_local_path_to_sys_path(local_repo_path)