From 2b6f9f616281d1631d47b1c1c8cf2d18fc508583 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 03:17:43 +0000 Subject: [PATCH] Tolerate coarse filesystem mtime storage in download -e refresh download -e refresh compares the mtime recorded for an asset against the mtime read back from the local file -- a value dandi itself set via os.utime() at the end of the previous download, making the comparison a filesystem round trip. It was performed with is_same_time()'s default tolerance of one microsecond, so on filesystems that do not store sub-second mtimes (mounted Windows volumes, FAT/exFAT, some network mounts) the comparison never succeeded and every asset of every dandiset was redownloaded on every invocation. Compare with a fixed tolerance of 2 s instead -- the coarsest mtime granularity a filesystem is known to use (FAT) -- following the same approach as rsync's --modify-window. 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. Also report both timestamps, their delta and the tolerance applied in the debug message logged when a refresh skip is rejected, so that this class of problem is diagnosable from a log. Fixes #1907. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01TarBMUbTumRRVoSSgRoDCs --- dandi/download.py | 35 +++++++++++- dandi/tests/fixtures.py | 30 ++++++++++ dandi/tests/test_download.py | 103 ++++++++++++++++++++++++++++++++++- 3 files changed, 165 insertions(+), 3 deletions(-) diff --git a/dandi/download.py b/dandi/download.py index ff4e172a6..6c416e039 100644 --- a/dandi/download.py +++ b/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} diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index af6d567ac..5f0ef908b 100644 --- a/dandi/tests/fixtures.py +++ b/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") diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index 731244504..335948b59 100644 --- a/dandi/tests/test_download.py +++ b/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