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