Create temp file in target directory to prevent cross-device link error

fsspec/implementations/local.py

@@ -399,7 +399,10 @@ def _open(self):
                     self.f = compress(self.f, mode=self.mode)
             else:
                 # TODO: check if path is writable?
-                i, name = tempfile.mkstemp()
+                i, name = tempfile.mkstemp(
+                    dir=os.path.dirname(self.path) or None,
+                    prefix=os.path.basename(self.path) + "-",
+                )
                 os.close(i)  # we want normal open and normal buffered file
                 self.temp = name
                 self.f = open(name, mode=self.mode)
@@ -438,22 +441,12 @@ def __getstate__(self):
     def commit(self):
         if self.autocommit:
             raise RuntimeError("Can only commit if not already set to autocommit")
+        shutil.move(self.temp, self.path)
         try:
-            shutil.move(self.temp, self.path)
-        except PermissionError as e:
-            # shutil.move raises PermissionError if os.rename
-            # and the default copy2 fallback with shutil.copystats fail.
-            # The file should be there nonetheless, but without copied permissions.
-            # If it doesn't exist, there was no permission to create the file.
-            if not os.path.exists(self.path):
-                raise e
-        else:
-            # If PermissionError is not raised, permissions can be set.
-            try:
-                mask = 0o666
-                os.chmod(self.path, mask & ~get_umask(mask))
-            except RuntimeError:
-                pass
+            mask = 0o666
+            os.chmod(self.path, mask & ~get_umask(mask))
+        except (RuntimeError, PermissionError):
+            pass
 
     def discard(self):
         if self.autocommit:

fsspec/implementations/tests/test_local.py

@@ -1,5 +1,4 @@
 import bz2
-import errno
 import gzip
 import os
 import os.path
@@ -563,24 +562,22 @@ def test_multiple_filesystems_use_umask_cache(tmpdir):
     assert get_umask.cache_info().hits == 1
 
 
-def test_transaction_cross_device_but_mock_temp_dir_on_wrong_device(tmpdir):
-    # If the temporary file for a transaction is not on the correct device,
-    # os.rename in shutil.move will raise EXDEV and lookup('chmod') will raise
-    # a PermissionError.
+def test_transaction_temp_file_in_target_dir(tmpdir):
+    # Temporary file should be created in the same directory as the target
+    # to avoid cross-device link errors when committing.
     fs = LocalFileSystem()
-    with (
-        patch(
-            "os.rename",
-            side_effect=OSError(errno.EXDEV, "Invalid cross-device link"),
-        ),
-        patch(
-            "os.chmod",
-            side_effect=PermissionError("Operation not permitted"),
-        ),
-    ):
-        with fs.transaction, fs.open(tmpdir + "/afile", "wb") as f:
+    target = str(tmpdir) + "/subdir/file.txt"
+    os.makedirs(os.path.dirname(target))
+
+    with fs.transaction:
+        with fs.open(target, "wb") as f:
+            assert os.path.dirname(f.temp) == os.path.dirname(target)
+            assert os.path.basename(f.temp).startswith("file.txt-")
             f.write(b"data")
 
+    with fs.open(target, "rb") as f:
+        assert f.read() == b"data"
+
 
 def test_make_path_posix():
     cwd = os.getcwd()