Fix writing on remote file systems with attribute cache (#1574)

Co-authored-by: Zachary Sailer <[email protected]>

Author: krassowski

.github/workflows/python-tests.yml

@@ -129,6 +129,24 @@ jobs:
         run: |
           hatch run test:nowarn || hatch -v run test:nowarn --lf
 
+  test_filesystems:
+    name: Test remote file systems
+    runs-on: ubuntu-latest
+    timeout-minutes: 20
+    steps:
+      - uses: actions/checkout@v5
+      - uses: jupyterlab/maintainer-tools/.github/actions/base-setup@v1
+      - name: Create NFS file system
+        run: |
+          sudo apt-get install -y nfs-kernel-server
+          mkdir /tmp/nfs_source /tmp/nfs_mount
+          echo "/tmp/nfs_source localhost(rw)" | sudo bash -c 'cat - > /etc/exports'
+          sudo exportfs -a
+          sudo mount -t nfs -o acregmin=60 localhost:/tmp/nfs_source /tmp/nfs_mount
+      - name: Run the tests
+        run: |
+          hatch run test:nowarn -k test_atomic_writing_permission_cache
+
   make_sdist:
     name: Make SDist
     runs-on: ubuntu-latest

jupyter_server/services/contents/fileio.py

@@ -39,8 +39,22 @@ def copy2_safe(src, dst, log=None):
 
     like shutil.copy2, but log errors in copystat instead of raising
     """
+    is_writable = os.access(src, os.W_OK)
+
+    if not is_writable:
+        # attempt to refresh the attribute cache (used by remote file systems)
+        # rather than raising a permission error before any operation that could
+        # refresh the attribute cache is allowed to take place.
+        fd = os.open(src, os.O_RDONLY)
+        try:
+            os.fsync(fd)
+        finally:
+            os.close(fd)
+        # re-try
+        is_writable = os.access(src, os.W_OK)
+
     # if src file is not writable, avoid creating a back-up
-    if not os.access(src, os.W_OK):
+    if not is_writable:
         if log:
             log.debug("Source file, %s, is not writable", src)
         raise PermissionError(errno.EACCES, f"File is not writable: {src}")

tests/services/contents/test_fileio.py

@@ -1,8 +1,11 @@
+import contextlib
 import json
 import logging
 import os
+import pathlib
 import stat
 import sys
+import tempfile
 
 import pytest
 from nbformat import validate
@@ -156,6 +159,47 @@ def test_atomic_writing_in_readonly_dir(tmp_path):
     assert mode == 0o500
 
 
+@contextlib.contextmanager
+def tmp_dir(tmp_root: pathlib.Path):
+    """Thin wrapper around `TemporaryDirectory` adopting it to `pathlib.Path`s"""
+    # we need to append `/` if we want to get a sub-directory
+    prefix = str(tmp_root) + "/"
+    with tempfile.TemporaryDirectory(prefix=prefix) as temp_path:
+        yield pathlib.Path(temp_path)
+
+
+@pytest.mark.skipif(
+    not pathlib.Path("/tmp/nfs_mount").exists(), reason="requires a local NFS mount"
+)
+def test_atomic_writing_permission_cache():
+    remote_source = pathlib.Path("/tmp/nfs_source")
+    local_mount = pathlib.Path("/tmp/nfs_mount")
+
+    with tmp_dir(tmp_root=local_mount) as local_mount_path:
+        f = local_mount_path / "file.txt"
+
+        # write initial content
+        f.write_text("original content")
+
+        # make the file non-writable
+        f.chmod(0o500)
+
+        # attempt write, should fail due to NFS attribute cache
+        with pytest.raises(PermissionError):
+            with atomic_writing(str(f)) as ff:
+                ff.write("new content")
+
+        source_path = remote_source / local_mount_path.name / "file.txt"
+
+        # make it readable by modifying attributes at source
+        source_path.chmod(0o700)
+
+        with atomic_writing(str(f)) as ff:
+            ff.write("new content")
+
+        assert f.read_text() == "new content"
+
+
 @pytest.mark.skipif(os.name == "nt", reason="test fails on Windows")
 def test_file_manager_mixin(tmp_path):
     mixin = FileManagerMixin()