Repository: fsspec/filesystem_spec

Issue:
Invalid cross-device link
Over at https://github.com/Lightning-AI/pytorch-lightning/issues/20270 (full traceback: https://github.com/Lightning-AI/pytorch-lightning/issues/20270#issuecomment-2829581394) the following error is raised

```python
OSError: [Errno 18] Invalid cross-device link: '/tmp/tmp657zwu1c' -> '/path/to/checkpoint.ckpt'
```
and during handling of the above exception
```python
PermissionError: [Errno 1] Operation not permitted
```
if trying to save checkpoints via lightning's `_atomic_save` function that now uses a transaction whereas there were no issues before the transaction was used.

An example of a source and target device interaction is saving training checkpoints from a compute node to a slow storage device.

Discussion:
@siemdejong: Interestingly, the checkpoint file is created at the destination with permissions
```
-rwxrwxrwx 1 nobody nogroup 573648714 Apr 25 08:39 '/path/to/checkpoint.ckpt'
```@siemdejong: Would it be acceptable to test for the source and destination to exist on the same device and if so, use `shutil.copyfile` instead of the default `copy_function` in `shutil.move(src, dst, copy_function=shutil.copy2)`. And do you think this would solve the relevant issue?

`shutil.copyfile` does not copy metadata and file permissions, while `shutil.copy2` does.@martindurant: > test for the source and destination to exist on the same device

Is there any reasonable way to do that? Or would you advocate simply trying with copy2 and falling back in the case that files?@siemdejong: The following might be able to achieve this via `os.stat(path).st_dev`, but I'm not sure this if this is exactly what one would expect.

```python
@lru_cache
def device_for_path(path: str) -> int:
    """
    Recursively find the st_dev of the nearest existing ancestor of `path`.
    Uses LRU cache to avoid repeated os.stat calls on the same paths.
    """
    if os.path.exists(path):
        return os.stat(path).st_dev
    return device_for_path(os.path.dirname(path))

def paths_on_same_device(path1, path2):
    """
    Return True if path1 and path2 would reside on the same filesystem device,
    even if they don’t actually exist yet.
    """
    return device_for_path(path1) == device_for_path(path2)
```

I would say check if paths are on the same device before using copy2 as it would do a copy (with copyfile) and after that fail with a copystat, leaving a copied file. But I'm not sure how much overhead this device check would have.

https://github.com/python/cpython/blob/e8cf3a1a641d6fa0bfa84a4f8363ff1e42abda30/Lib/shutil.py#L522-L523

Another approach then might be to manually apply `copyfile` first and then `copystats` second in `LocalFileOpener.commit()` and cache a possible error, but that would result in a file on a remote device with permissions that may differ from the permissions it would have on the local device, hence my question if that is acceptable.

The easiest solution would indeed be to just do shutil.move, let it decide to use copy2 in case of exdev error and catch the permission error copystat would throw, but this may be a breaking change.

The copy_function arg could also be exposed in some way so that packages using fsspec can decide for themselves.@martindurant: Your function I think would still result in many many calls when processing a lot of files, and I imagine is problematic on windows at least (if permissions work there at all!). 

> manually apply copyfile first and then copystats second in LocalFileOpener.commit()

It's actually probably the best we can do, since some mountable devices don't support a full permissioning model anyway. See also #1826 for applying the default "umask" to temporary files on commit.@siemdejong: Can I contribute with a PR here?@martindurant: Yes, please do

Solution patch:
diff --git a/fsspec/implementations/local.py b/fsspec/implementations/local.py
index 70dfd90..0152816 100644
--- a/fsspec/implementations/local.py
+++ b/fsspec/implementations/local.py
@@ -430,12 +430,22 @@ class LocalFileOpener(io.IOBase):
     def commit(self):
         if self.autocommit:
             raise RuntimeError("Can only commit if not already set to autocommit")
-        shutil.move(self.temp, self.path)
         try:
-            mask = 0o666
-            os.chmod(self.path, mask & ~get_umask(mask))
-        except RuntimeError:
-            pass
+            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
 
     def discard(self):
         if self.autocommit:


Test patch:
diff --git a/fsspec/implementations/tests/test_local.py b/fsspec/implementations/tests/test_local.py
index 2e4c07c..fd6b656 100644
--- a/fsspec/implementations/tests/test_local.py
+++ b/fsspec/implementations/tests/test_local.py
@@ -1,4 +1,5 @@
 import bz2
+import errno
 import gzip
 import os
 import os.path
@@ -562,6 +563,25 @@ 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.
+    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:
+            f.write(b"data")
+
+
 def test_make_path_posix():
     cwd = os.getcwd()
     if WIN: