Decide the refresh mtime match from the stored value, not a fixed window

dandi/consts.py

@@ -258,11 +258,15 @@ 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
+#: Granularities (in nanoseconds) with which filesystems are known to store
+#: mtimes: FAT rounds to a multiple of two seconds, ext3/HFS+/ISO9660 truncate
+#: to whole seconds, and exFAT to 10 ms.  Used under ``-e refresh`` to decide
+#: whether the mtime read back from a downloaded file is a plausible
+#: quantization of the one we set on it -- see `is_same_mtime()`.
+MTIME_GRANULARITIES_NS = (2_000_000_000, 1_000_000_000, 10_000_000)
+
+#: Slack (in nanoseconds) allowed for the float round trip itself:
+#: ``os.utime()`` takes the time as seconds in a C double, so even a filesystem
+#: storing mtimes at full nanosecond resolution reads back a few tens of
+#: nanoseconds away from what we asked for.
+MTIME_ROUNDTRIP_SLACK_NS = 1_000

dandi/download.py

@@ -40,7 +40,7 @@
 from . import get_logger
 from .consts import (
     DOWNLOAD_SUFFIX,
-    MTIME_TOLERANCE,
+    MTIME_GRANULARITIES_NS,
     RETRY_STATUSES,
     SyncMode,
     dandiset_metadata_file,
@@ -66,7 +66,7 @@
     exclude_from_zarr,
     flattened,
     get_retry_after,
-    is_same_time,
+    is_same_mtime,
     path_is_subpath,
     pluralize,
     yaml_load,
@@ -564,6 +564,20 @@ def _skip_file(msg: Any, **kwargs: Any) -> dict:
     return {"status": "skipped", "message": str(msg), **kwargs}
 
 
+def _is_local_file_current(path: str, record: datetime) -> bool:
+    """Is the local file at least as new as the record?
+
+    For a file whose mtime we set ourselves with `os.utime()`: a destination
+    that quantizes mtimes reads it back below the value written, and a bare
+    ``>=`` takes that for an older file and overwrites a copy the user may
+    have edited; `is_same_mtime()` recognizes it as the value we wrote.
+    Kept one-sided: a genuinely newer local copy is current too, which an
+    equality test alone would reject.
+    """
+    st = os.lstat(path)
+    return st.st_mtime >= record.timestamp() or is_same_mtime(st.st_mtime_ns, record)
+
+
 def _populate_dandiset_yaml(
     dandiset_path: str | Path, dandiset: RemoteDandiset, existing: DownloadExisting
 ) -> Iterator[dict]:
@@ -591,7 +605,7 @@ def _populate_dandiset_yaml(
             raise RuntimeError("Not refreshing path in git annex repository")
         elif existing is DownloadExisting.SKIP or (
             existing is DownloadExisting.REFRESH
-            and os.lstat(dandiset_yaml).st_mtime >= mtime.timestamp()
+            and _is_local_file_current(dandiset_yaml, mtime)
         ):
             yield _skip_file("already exists")
             return
@@ -708,13 +722,7 @@ def _download_file(
             else:
                 stat = os.stat(op.realpath(path))
                 same = []
-                # 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):
+                if is_same_mtime(stat.st_mtime_ns, mtime):
                     same.append("mtime")
                 if size is not None and stat.st_size == size:
                     same.append("size")
@@ -724,18 +732,16 @@ def _download_file(
                     # TODO: add recording and handling of .nwb object_id
                     yield _skip_file("same time and size", size=size)
                     return
-                # 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",
+                    "granularities: %s ns, 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,
+                    MTIME_GRANULARITIES_NS,
                     stat.st_size,
                     size,
                 )

dandi/tests/test_download.py

@@ -31,7 +31,12 @@
 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, MTIME_TOLERANCE, SyncMode, dandiset_metadata_file
+from ..consts import (
+    DRAFT,
+    MTIME_GRANULARITIES_NS,
+    SyncMode,
+    dandiset_metadata_file,
+)
 from ..dandiarchive import DandisetURL
 from ..download import (
     DownloadDirectory,
@@ -43,11 +48,12 @@
     PYOUTHelper,
     _check_attempts_and_sleep,
     _download_file,
+    _is_local_file_current,
     download,
 )
 from ..exceptions import NotFoundError
 from ..support.digests import Digester
-from ..utils import list_paths, yaml_load
+from ..utils import is_same_mtime, list_paths, yaml_load
 
 
 # both urls point to 000027 (lean test dataset), and both draft and "released"
@@ -194,14 +200,16 @@ 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
+        # nanoseconds (`ns`, used by e.g. `shutil.copystat()`), never both.
+        # Quantize in integer nanoseconds -- as a filesystem does, and so that
+        # granularities not exactly representable as a float (10 ms) truncate
+        # cleanly -- then forward whichever form was given.
         if granularity:
+            g_ns = round(granularity * 10**9)
             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)
+                ns, times = tuple(round(t * 10**9) for t in times), None
+            assert ns is not None
+            ns = tuple(n // g_ns * g_ns for n in ns)
         if ns is not None:
             real_utime(path, ns=ns, **kwargs)
         else:
@@ -223,7 +231,7 @@ def set_granularity(value: float) -> None:
 
 
 @pytest.mark.ai_generated
-@pytest.mark.parametrize("granularity", [0.0, 1.0, 2.0])
+@pytest.mark.parametrize("granularity", [0.0, 0.01, 1.0, 2.0])
 def test_download_file_refresh_coarse_mtime_fs(
     tmp_path: Path, coarse_mtime_fs: Callable[[float], None], granularity: float
 ) -> None:
@@ -310,13 +318,87 @@ def downloader(start_at: int = 0) -> Iterator[bytes]:
         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
+    assert f"granularities: {MTIME_GRANULARITIES_NS} ns" 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
 
 
+#: Same size as `COARSE_MTIME_CONTENT`, different bytes -- an asset replaced
+#: in place, which only the mtime can distinguish from the original
+COARSE_MTIME_CONTENT_2 = b"This is other text\n"
+
+
+@pytest.mark.ai_generated
+def test_download_file_refresh_detects_subsecond_change(tmp_path: Path) -> None:
+    """A sub-second change on a filesystem that stores mtimes exactly must
+    still be redownloaded.
+
+    The counterpart to `test_download_file_refresh_coarse_mtime_fs`: tolerating
+    the coarsest known granularity unconditionally would skip this, since the
+    size is unchanged and the mtime moved well under two seconds.  Here the
+    filesystem stored the previous mtime at full precision, so the discrepancy
+    is not something its quantization could have produced.
+    """
+    assert len(COARSE_MTIME_CONTENT_2) == len(COARSE_MTIME_CONTENT)
+    path = tmp_path / "file.txt"
+
+    def download_it(content: bytes, mtime: datetime) -> list[dict]:
+        def downloader(start_at: int = 0) -> Iterator[bytes]:
+            yield content[start_at:]
+
+        return list(
+            _download_file(
+                downloader,
+                path,
+                tmp_path,
+                Lock(),
+                size=len(content),
+                mtime=mtime,
+                existing=DownloadExisting.REFRESH,
+            )
+        )
+
+    download_it(COARSE_MTIME_CONTENT, COARSE_MTIME_RECORD)
+    assert path.read_bytes() == COARSE_MTIME_CONTENT
+    # The asset was replaced 0.4 s later with different bytes of the same size
+    statuses = download_it(
+        COARSE_MTIME_CONTENT_2, COARSE_MTIME_RECORD + timedelta(seconds=0.4)
+    )
+    assert {"status": "downloading"} in statuses
+    assert path.read_bytes() == COARSE_MTIME_CONTENT_2
+
+
+@pytest.mark.ai_generated
+@pytest.mark.parametrize("granularity", [0.0, 0.01, 1.0, 2.0])
+def test_is_local_file_current_coarse_mtime_fs(
+    tmp_path: Path, coarse_mtime_fs: Callable[[float], None], granularity: float
+) -> None:
+    """A file we just wrote counts as current however coarsely the filesystem
+    stored the mtime, while genuinely older/newer copies compare as themselves.
+    """
+    coarse_mtime_fs(granularity)
+    path = tmp_path / "file.txt"
+    path.write_text("some content\n")
+    os.utime(path, (time.time(), COARSE_MTIME_RECORD.timestamp()))
+
+    # The mtime we just set, read back through the filesystem
+    assert _is_local_file_current(str(path), COARSE_MTIME_RECORD)
+    # The record moved on by more than any granularity -> stale, rewrite it
+    assert not _is_local_file_current(
+        str(path), COARSE_MTIME_RECORD + timedelta(hours=1)
+    )
+    # A locally edited copy is newer than the record and must be left alone.
+    # This is why the check stays one-sided: `is_same_mtime()` on its own would
+    # call an hour-newer file "not the record" and clobber the user's edit.
+    os.utime(
+        path, (time.time(), (COARSE_MTIME_RECORD + timedelta(hours=1)).timestamp())
+    )
+    assert _is_local_file_current(str(path), COARSE_MTIME_RECORD)
+    assert not is_same_mtime(path.stat().st_mtime_ns, COARSE_MTIME_RECORD)
+
+
 def test_download_newest_version(text_dandiset: SampleDandiset, tmp_path: Path) -> None:
     dandiset = text_dandiset.dandiset
     dandiset_id = text_dandiset.dandiset_id

dandi/tests/test_utils.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 from collections.abc import Iterable
+import datetime
 import inspect
 import logging
 import os.path as op
@@ -28,6 +29,7 @@
     get_module_version,
     get_utcnow_datetime,
     is_page2_url,
+    is_same_mtime,
     is_same_time,
     is_url,
     on_windows,
@@ -158,6 +160,57 @@ def test_time_samples(t: str) -> None:
     )  # exactly the same
 
 
+#: An 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
+MTIME_RECORD = datetime.datetime(
+    2026, 8, 22, 15, 21, 21, 651000, tzinfo=datetime.timezone.utc
+)
+
+
+def _ns(t: datetime.datetime) -> int:
+    return int(t.timestamp() * 10**9)
+
+
+def _truncated(t: datetime.datetime, granularity_ns: int) -> int:
+    """`t` as a filesystem with the given mtime granularity would store it"""
+    return _ns(t) // granularity_ns * granularity_ns
+
+
+@pytest.mark.ai_generated
+@pytest.mark.parametrize(
+    "local_ns,record,same",
+    [
+        # An unchanged file, however the filesystem stored the mtime we set:
+        # exactly, or truncated to exFAT's 10 ms, a whole second, or FAT's 2 s
+        (_ns(MTIME_RECORD), MTIME_RECORD, True),
+        (_truncated(MTIME_RECORD, 10**7), MTIME_RECORD, True),
+        (_truncated(MTIME_RECORD, 10**9), MTIME_RECORD, True),
+        (_truncated(MTIME_RECORD, 2 * 10**9), MTIME_RECORD, True),
+        # A changed file, against a stored value too precise to explain the gap
+        (_ns(MTIME_RECORD), MTIME_RECORD + datetime.timedelta(seconds=0.4), False),
+        (_ns(MTIME_RECORD), MTIME_RECORD + datetime.timedelta(seconds=1.9), False),
+        (
+            _truncated(MTIME_RECORD, 10**7),
+            MTIME_RECORD + datetime.timedelta(seconds=0.4),
+            False,
+        ),
+        # A gap wider than any known granularity, whatever the stored value
+        (
+            _truncated(MTIME_RECORD, 2 * 10**9),
+            MTIME_RECORD + datetime.timedelta(seconds=1.5),
+            False,
+        ),
+        (
+            _ns(MTIME_RECORD - datetime.timedelta(hours=1)),
+            MTIME_RECORD,
+            False,
+        ),
+    ],
+)
+def test_is_same_mtime(local_ns: int, record: datetime.datetime, same: bool) -> None:
+    assert is_same_mtime(local_ns, record) is same
+
+
 def test_flatten() -> None:
     assert inspect.isgenerator(flatten([1]))
     # flattened is just a list() around flatten

dandi/utils.py

@@ -35,7 +35,13 @@
 from yarl import URL
 
 from . import __version__, get_logger
-from .consts import DandiInstance, known_instances, known_instances_rev
+from .consts import (
+    MTIME_GRANULARITIES_NS,
+    MTIME_ROUNDTRIP_SLACK_NS,
+    DandiInstance,
+    known_instances,
+    known_instances_rev,
+)
 from .exceptions import BadCliVersionError, CliVersionTooOldError
 
 AnyPath = Union[str, Path]
@@ -157,6 +163,51 @@ def is_same_time(
     )
 
 
+_EPOCH = datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc)
+
+
+def _datetime_to_ns(dt: datetime.datetime) -> int:
+    """Epoch nanoseconds for `dt`, without a float round trip"""
+    delta = dt - _EPOCH
+    return (delta.days * 86400 + delta.seconds) * 10**9 + delta.microseconds * 1000
+
+
+def is_same_mtime(local_ns: int, record: datetime.datetime) -> bool:
+    """Is `local_ns` the mtime `record`, as a filesystem could have stored it?
+
+    For an mtime we set ourselves with `os.utime()` and are now reading back,
+    where the only discrepancy an otherwise-unchanged file can show is the
+    filesystem's own quantization of the value.  Where `is_same_time()`
+    tolerates a fixed window -- which on a filesystem that stores the value
+    exactly would mask a real change -- this requires the discrepancy to be one
+    the *observed* value can account for: `local_ns` must be a multiple of some
+    granularity in `MTIME_GRANULARITIES_NS` that is itself wider than the gap.
+    A filesystem truncating to whole seconds cannot have produced a stored
+    value of ``...21.651``, so against such a value even a millisecond of drift
+    means the file really did change; a stored ``...20.000`` is consistent with
+    truncation, and a gap of up to two seconds says nothing either way.
+
+    Parameters
+    ----------
+    local_ns: int
+      The mtime as read back from the filesystem, in epoch nanoseconds, i.e.
+      `os.stat()`'s `st_mtime_ns`.  Nanoseconds rather than `st_mtime` so that
+      the multiple-of test is exact.
+    record: datetime.datetime
+      The mtime we asked for.
+
+    See https://github.com/dandi/dandi-cli/issues/1907
+    """
+    delta = abs(_datetime_to_ns(record) - local_ns)
+    if delta <= MTIME_ROUNDTRIP_SLACK_NS:
+        return True
+    # TODO: this accounts for quantization only.  A destination that also
+    # *shifts* mtimes -- notably FAT, which stores local time, so every mtime
+    # moves by an hour across a DST transition -- will still compare unequal
+    # once after such a shift.  Niche enough to leave for now.
+    return any(g > delta and local_ns % g == 0 for g in MTIME_GRANULARITIES_NS)
+
+
 def ensure_strtime(
     t: str | int | float | datetime.datetime, isoformat: bool = True
 ) -> str: