Tolerate coarse filesystem mtimes in download -e refresh with a fixed 2 s window

dandi/download.py

@@ -68,6 +68,21 @@
 
 lgr = get_logger()
 
+#: Tolerance (in seconds) for comparing a local file's mtime against the mtime
+#: recorded for the asset when deciding whether ``DownloadExisting.REFRESH``
+#: may skip a redownload.  The local mtime is one we set ourselves via
+#: `os.utime()` at the end of the previous download, so the comparison is
+#: really a filesystem round trip — and not every filesystem stores mtimes at
+#: the resolution `os.stat()` reports them at: mounted Windows volumes,
+#: FAT/exFAT and some network filesystems truncate or round the value, by up
+#: to FAT's 2 s granularity.  Tolerate that coarsest known granularity
+#: everywhere (cf. rsync's ``--modify-window``) instead of assuming the value
+#: round-trips exactly.  A skip additionally requires an exact size match, and
+#: the recorded mtime only moves when the asset is actually replaced, so the
+#: widened window cannot in practice mistake an updated asset for an unchanged
+#: one.  https://github.com/dandi/dandi-cli/issues/1907
+REFRESH_MTIME_TOLERANCE = 2.0
+
 
 class DownloadExisting(StrEnum):
     ERROR = "error"
@@ -702,7 +717,9 @@ def _download_file(
             else:
                 stat = os.stat(op.realpath(path))
                 same = []
-                if is_same_time(stat.st_mtime, mtime):
+                if is_same_time(
+                    stat.st_mtime, mtime, tolerance=REFRESH_MTIME_TOLERANCE
+                ):
                     same.append("mtime")
                 if size is not None and stat.st_size == size:
                     same.append("size")
@@ -712,7 +729,21 @@ def _download_file(
                     # TODO: add recording and handling of .nwb object_id
                     yield _skip_file("same time and size", size=size)
                     return
-                lgr.debug(f"{path!r} - same attributes: {same}.  Redownloading")
+                lgr.debug(
+                    "%r - same attributes: %s.  Redownloading.  "
+                    "Local mtime: %s (%.6f), record mtime: %s (%.6f), "
+                    "delta: %.6f s, tolerance: %g s, local size: %s, record size: %s",
+                    str(path),
+                    same,
+                    ensure_datetime(stat.st_mtime),
+                    stat.st_mtime,
+                    mtime,
+                    mtime.timestamp(),
+                    abs(stat.st_mtime - mtime.timestamp()),
+                    REFRESH_MTIME_TOLERANCE,
+                    stat.st_size,
+                    size,
+                )
 
     if size is not None:
         yield {"size": size}

dandi/tests/fixtures.py

@@ -71,6 +71,36 @@ def capture_all_logs(caplog: pytest.LogCaptureFixture) -> None:
     caplog.set_level(logging.DEBUG, logger="dandi")
 
 
+@pytest.fixture()
+def coarse_mtime_fs(
+    monkeypatch: pytest.MonkeyPatch,
+) -> Iterator[Callable[[float], None]]:
+    """Simulate a filesystem that does not store sub-second mtimes
+
+    Yields a callable taking the granularity (in seconds) with which mtimes
+    should henceforth be stored; until it is called, mtimes are stored as
+    given.  This is done by patching `os.utime()` to quantize the times it is
+    asked to set, which is what filesystems such as those on mounted Windows
+    volumes, FAT and exFAT effectively do — real (ext4/XFS/tmpfs) temporary
+    directories store mtimes with nanosecond resolution and so cannot exercise
+    this behavior.
+    """
+    real_utime = os.utime
+    granularity = 0.0
+
+    def quantizing_utime(path: Any, times: Any = None, **kwargs: Any) -> None:
+        if granularity and times is not None:
+            times = tuple(t // granularity * granularity for t in times)
+        real_utime(path, times, **kwargs)
+
+    def set_granularity(value: float) -> None:
+        nonlocal granularity
+        granularity = value
+
+    monkeypatch.setattr(os, "utime", quantizing_utime)
+    yield set_granularity
+
+
 # TODO: move into some common fixtures.  We might produce a number of files
 #       and also carry some small ones directly in git for regression testing
 @pytest.fixture(scope="session")

dandi/tests/test_download.py

@@ -1,7 +1,8 @@
 from __future__ import annotations
 
-from collections.abc import Callable
+from collections.abc import Callable, Iterator
 from contextlib import nullcontext
+from datetime import datetime, timedelta, timezone
 from email.utils import parsedate_to_datetime
 from functools import partial
 from glob import glob
@@ -13,6 +14,7 @@
 from pathlib import Path
 import re
 from shutil import rmtree
+from threading import Lock
 import time
 from unittest import mock
 
@@ -39,6 +41,7 @@
     ProgressCombiner,
     PYOUTHelper,
     _check_attempts_and_sleep,
+    _download_file,
     download,
 )
 from ..exceptions import NotFoundError
@@ -170,6 +173,104 @@ def test_download_000027_resume(
         assert digester(str(nwb)) == digests
 
 
+#: An arbitrary asset mtime with a non-zero sub-second component, i.e. one that
+#: does not survive a round trip through a filesystem storing whole seconds only
+COARSE_MTIME_RECORD = datetime(2026, 8, 22, 15, 21, 20, 651000, tzinfo=timezone.utc)
+
+COARSE_MTIME_CONTENT = b"This is test text.\n"
+
+
+@pytest.mark.ai_generated
+@pytest.mark.parametrize("granularity", [0.0, 1.0, 2.0])
+def test_download_file_refresh_coarse_mtime_fs(
+    tmp_path: Path, coarse_mtime_fs: Callable[[float], None], granularity: float
+) -> None:
+    """`existing=refresh` must skip an unchanged file no matter how coarsely the
+    filesystem stores the mtime that we ourselves set after downloading it.
+
+    Regression test for https://github.com/dandi/dandi-cli/issues/1907 , where
+    every file of a dandiset was redownloaded on every run whenever the
+    destination did not store sub-second mtimes (a mounted Windows volume,
+    FAT/exFAT, some network filesystems).
+    """
+    coarse_mtime_fs(granularity)
+    path = tmp_path / "file.txt"
+    downloads = 0
+
+    def downloader(start_at: int = 0) -> Iterator[bytes]:
+        nonlocal downloads
+        downloads += 1
+        yield COARSE_MTIME_CONTENT[start_at:]
+
+    def download_it(existing: DownloadExisting) -> list[dict]:
+        return list(
+            _download_file(
+                downloader,
+                path,
+                tmp_path,
+                Lock(),
+                size=len(COARSE_MTIME_CONTENT),
+                mtime=COARSE_MTIME_RECORD,
+                existing=existing,
+            )
+        )
+
+    assert {"status": "setting mtime"} in download_it(DownloadExisting.ERROR)
+    assert path.read_bytes() == COARSE_MTIME_CONTENT
+    assert downloads == 1
+
+    # The refresh pass must not transfer anything at all, however coarsely the
+    # filesystem happened to store the mtime just set
+    downloads = 0
+    assert download_it(DownloadExisting.REFRESH) == [
+        {
+            "status": "skipped",
+            "message": "same time and size",
+            "size": len(COARSE_MTIME_CONTENT),
+        }
+    ]
+    assert downloads == 0
+
+
+@pytest.mark.ai_generated
+def test_download_file_refresh_reports_mtime_mismatch(
+    tmp_path: Path,
+    coarse_mtime_fs: Callable[[float], None],
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    """A genuinely out-of-date file is still redownloaded, and the rejected skip
+    reports both timestamps, their delta, and the tolerance applied."""
+    coarse_mtime_fs(1.0)
+    path = tmp_path / "file.txt"
+    path.write_bytes(COARSE_MTIME_CONTENT)
+    # The local copy is an hour older than the record
+    stale = COARSE_MTIME_RECORD - timedelta(hours=1)
+    os.utime(path, (time.time(), stale.timestamp()))
+
+    def downloader(start_at: int = 0) -> Iterator[bytes]:
+        yield COARSE_MTIME_CONTENT[start_at:]
+
+    statuses = list(
+        _download_file(
+            downloader,
+            path,
+            tmp_path,
+            Lock(),
+            size=len(COARSE_MTIME_CONTENT),
+            mtime=COARSE_MTIME_RECORD,
+            existing=DownloadExisting.REFRESH,
+        )
+    )
+    assert {"status": "downloading"} in statuses
+
+    (msg,) = [
+        r.getMessage() for r in caplog.records if "Redownloading" in r.getMessage()
+    ]
+    assert "same attributes: ['size']" in msg
+    assert "delta: 3600.65" in msg
+    assert "tolerance: 2 s" in msg
+
+
 def test_download_newest_version(text_dandiset: SampleDandiset, tmp_path: Path) -> None:
     dandiset = text_dandiset.dandiset
     dandiset_id = text_dandiset.dandiset_id