Keep lock files to prevent rare concurrency issue

The lock file could be removed when the 2nd process gets the lock.
And then the 3rd process will lock on a different lock file handle.
Although the lock path stays the same.

1st proc gets the lock
2nd proc waits for the lock
1st proc releases the lock
1st proc removes the lock file
2nd proc gets the lock
3rd proc creates a new lock file and gets the lock

Windows doesn't have this problem.

This commit moves the lock files to a subdirectory of the hub cache,
and don't remove it after downloading.

The lock files are named with their 'etag' and '.lock' extension, placed
in the 'HUGGINGFACE_HUB_CACHE/.locks/repo_folder_name' directory. The
repo_folder_name is generated from 'repo_id' and 'repo_type', to avoid
same 'etag' in different repos.

Co-authored-by: Lucain <[email protected]>

Author: beeender

src/huggingface_hub/file_download.py

@@ -1197,6 +1197,7 @@ def hf_hub_download(
         cache_dir = str(cache_dir)
     if isinstance(local_dir, Path):
         local_dir = str(local_dir)
+    locks_dir = os.path.join(cache_dir, ".locks")
 
     if subfolder == "":
         subfolder = None
@@ -1410,7 +1411,8 @@ def hf_hub_download(
             return pointer_path
 
     # Prevent parallel downloads of the same file with a lock.
-    lock_path = blob_path + ".lock"
+    # etag could be duplicated across repos, 
+    lock_path = os.path.join(locks_dir, repo_folder_name(repo_id=repo_id, repo_type=repo_type), f"{etag}.lock")
 
     # Some Windows versions do not allow for paths longer than 255 characters.
     # In this case, we must specify it is an extended path by using the "\\?\" prefix.
@@ -1420,6 +1422,7 @@ def hf_hub_download(
     if os.name == "nt" and len(os.path.abspath(blob_path)) > 255:
         blob_path = "\\\\?\\" + os.path.abspath(blob_path)
 
+    Path(lock_path).parent.mkdir(parents=True, exist_ok=True)
     with FileLock(lock_path):
         # If the download just completed while the lock was activated.
         if os.path.exists(pointer_path) and not force_download:
@@ -1494,11 +1497,6 @@ def _resumable_file_manager() -> Generator[io.BufferedWriter, None, None]:
                 _chmod_and_replace(temp_file.name, local_dir_filepath)
             pointer_path = local_dir_filepath  # for return value
 
-    try:
-        os.remove(lock_path)
-    except OSError:
-        pass
-
     return pointer_path
 
 

tests/test_file_download.py

@@ -653,6 +653,20 @@ def test_download_from_a_gated_repo_with_hf_hub_download(self):
                     cache_dir=tmpdir,
                 )
 
+    @unittest.skipIf(os.name == "nt", "Lock files are always deleted on Windows.")
+    def test_keep_lock_file(self):
+        """Lock files should not be deleted on Linux."""
+        with SoftTemporaryDirectory() as tmpdir:
+            hf_hub_download(DUMMY_MODEL_ID, filename=CONFIG_NAME, cache_dir=tmpdir)
+            lock_file_exist = False
+            locks_dir = os.path.join(tmpdir, ".locks")
+            for subdir, dirs, files in os.walk(locks_dir):
+                for file in files:
+                    if file.endswith(".lock"):
+                        lock_file_exist = True
+                        break
+            self.assertTrue(lock_file_exist, "no lock file can be found")
+
 
 @with_production_testing
 @pytest.mark.usefixtures("fx_cache_dir")