diff --git a/dandi/consts.py b/dandi/consts.py index 338c96713..baca5465c 100644 --- a/dandi/consts.py +++ b/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 diff --git a/dandi/download.py b/dandi/download.py index ff4e172a6..e92aa0f7f 100644 --- a/dandi/download.py +++ b/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} diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index 731244504..3485d8031 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,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