Tolerate the filesystem's mtime granularity in download -e refresh

dandi/consts.py

@@ -257,3 +257,12 @@ def urls(self) -> Iterator[str]:
 
 #: Suffix used for temporary download directories
 DOWNLOAD_SUFFIX = ".dandidownload"
+
+#: Tolerance (in seconds) when comparing an asset's recorded mtime against the
+#: mtime read back from the downloaded file under ``-e refresh``.  That local
+#: mtime is one we set ourselves with ``os.utime()``, 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,
+#: exFAT and some network filesystems truncate, and FAT rounds to a multiple
+#: of two seconds.  See https://github.com/dandi/dandi-cli/issues/1907
+MTIME_TOLERANCE = 2.0

dandi/download.py

@@ -13,7 +13,7 @@
 from collections import Counter, deque
 from collections.abc import Callable, Iterable, Iterator, Sequence
 from dataclasses import InitVar, dataclass, field
-from datetime import datetime
+from datetime import datetime, timezone
 from enum import Enum, StrEnum
 from functools import partial
 import hashlib
@@ -38,7 +38,13 @@
 import requests
 
 from . import get_logger
-from .consts import DOWNLOAD_SUFFIX, RETRY_STATUSES, SyncMode, dandiset_metadata_file
+from .consts import (
+    DOWNLOAD_SUFFIX,
+    MTIME_TOLERANCE,
+    RETRY_STATUSES,
+    SyncMode,
+    dandiset_metadata_file,
+)
 from .dandiapi import AssetType, BaseRemoteZarrAsset, RemoteDandiset
 from .dandiarchive import (
     AssetItemURL,
@@ -702,7 +708,13 @@ def _download_file(
             else:
                 stat = os.stat(op.realpath(path))
                 same = []
-                if is_same_time(stat.st_mtime, mtime):
+                # The mtime compared against here is the one we set ourselves
+                # with os.utime() after the previous download, so this is
+                # really a filesystem round trip; tolerate the coarsest
+                # granularity filesystems are known to store mtimes with
+                # instead of assuming the value round-trips exactly.  See
+                # https://github.com/dandi/dandi-cli/issues/1907
+                if is_same_time(stat.st_mtime, mtime, tolerance=MTIME_TOLERANCE):
                     same.append("mtime")
                 if size is not None and stat.st_size == size:
                     same.append("size")
@@ -712,7 +724,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")
+                # Both timestamps are reported in UTC, which is what
+                # is_same_time() normalizes to before comparing them
+                lgr.debug(
+                    "%r - same attributes: %s.  Redownloading.  "
+                    "local mtime: %s, record mtime: %s, delta: %f s, "
+                    "tolerance: %g s, local size: %s, record size: %s",
+                    str(path),
+                    same,
+                    ensure_datetime(stat.st_mtime, tz=timezone.utc),
+                    ensure_datetime(mtime, tz=timezone.utc),
+                    abs(stat.st_mtime - mtime.timestamp()),
+                    MTIME_TOLERANCE,
+                    stat.st_size,
+                    size,
+                )
 
     if size is not None:
         yield {"size": size}

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,7 +14,9 @@
 from pathlib import Path
 import re
 from shutil import rmtree
+from threading import Lock
 import time
+from typing import Any
 from unittest import mock
 
 from dandischema.models import ID_PATTERN
@@ -28,7 +31,7 @@
 from .fixtures import SampleDandiset, SampleDandisetFactory
 from .skip import mark
 from .test_helpers import TWO_ARRAY_ZARR_LAYOUT, assert_dirtrees_eq, zarr_format_of
-from ..consts import DRAFT, SyncMode, dandiset_metadata_file
+from ..consts import DRAFT, MTIME_TOLERANCE, SyncMode, dandiset_metadata_file
 from ..dandiarchive import DandisetURL
 from ..download import (
     DownloadDirectory,
@@ -39,6 +42,7 @@
     ProgressCombiner,
     PYOUTHelper,
     _check_attempts_and_sleep,
+    _download_file,
     download,
 )
 from ..exceptions import NotFoundError
@@ -170,6 +174,149 @@ def test_download_000027_resume(
         assert digester(str(nwb)) == digests
 
 
+@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 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, *, ns: Any = None, **kwargs: Any
+    ) -> None:
+        # `os.utime()` accepts the times either as seconds (`times`) or as
+        # nanoseconds (`ns`, used by e.g. `shutil.copystat()`), never both, so
+        # quantize whichever was given and forward it the same way
+        if granularity:
+            if times is not None:
+                times = tuple(t // granularity * granularity for t in times)
+            if ns is not None:
+                ns_granularity = int(granularity * 1_000_000_000)
+                ns = tuple(n // ns_granularity * ns_granularity for n in ns)
+        if ns is not None:
+            real_utime(path, ns=ns, **kwargs)
+        else:
+            real_utime(path, times, **kwargs)
+
+    def set_granularity(value: float) -> None:
+        nonlocal granularity
+        granularity = value
+
+    monkeypatch.setattr(os, "utime", quantizing_utime)
+    yield set_granularity
+
+
+#: 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, 21, 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, i.e. out of date by far
+    # more than any filesystem's mtime granularity
+    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 f"tolerance: {MTIME_TOLERANCE:g} s" in msg
+    # The record's mtime is reported in full, so that someone reading the log
+    # can see what it was compared against.  The local one is not asserted on:
+    # it is whatever the filesystem stored, which is the point of the test.
+    assert f"record mtime: {COARSE_MTIME_RECORD}" in msg
+
+
 def test_download_newest_version(text_dandiset: SampleDandiset, tmp_path: Path) -> None:
     dandiset = text_dandiset.dandiset
     dandiset_id = text_dandiset.dandiset_id