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

[CodyCBakerPhD]

Closes #1907. Supersedes #1908.

Fix

The refresh branch of _download_file() compared the recorded mtime against the local
one with is_same_time()'s default 1 µs tolerance, i.e. it assumed the value round-trips
through the filesystem exactly. Pass a MTIME_TOLERANCE of 2 s instead.

#1908 fixed this by measuring each destination filesystem's granularity — 121 lines: a
temp file probed inside the user's download directory, a table of known granularities, an
st_dev-keyed cache behind a threading.Lock, and a shared fixture reaching into that
private cache so tests could clear it. Two properties of the comparison site make a
constant sufficient:

1. The local mtime is one this package wrote itself, a few lines below in the same function
   (os.utime(path, (time.time(), mtime.timestamp()))). The comparison is therefore
   only a filesystem round trip, and the sole error it must absorb is that filesystem's
   own quantization — worst case FAT's 2 s.
2. The skip also requires stat.st_size == size, so a wider tolerance can only mis-skip
   an asset that changed with an identical size and an mtime moving under 2 s.

🤖 Generated with Claude Code

https://claude.ai/code/session_015TCMNL9Jkrz4ooAf2bmSfE

[codecov]

Codecov Report

❌ Patch coverage is 93.54839% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.26%. Comparing base (9a9eb62) to head (32b7bab).

Files with missing lines	Patch %	Lines
dandi/tests/test_download.py	94.73%	3 Missing ⚠️
dandi/consts.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1910      +/-   ##
==========================================
+ Coverage   77.17%   77.26%   +0.08%     
==========================================
  Files          89       89              
  Lines       13208    13264      +56     
==========================================
+ Hits        10193    10248      +55     
- Misses       3015     3016       +1     

Flag	Coverage Δ
unittests	77.26% <93.54%> (+0.08%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[CodyCBakerPhD]

@yarikoptic Confirmed this solved my issue on 'my system'

[yarikoptic]

The fix is right, and choosing a constant over #1908's probe-and-cache machinery is well argued. Both properties in the description hold up against the code: the compared mtime is one _download_file() set itself at download.py:862, and the stat.st_size == size conjunct is a real backstop. Happy to see this go in once the test constant is fixed.

Verified locally: all four new tests pass; reverting just the tolerance= argument fails coarse_mtime_fs[1.0] and [2.0], so the regression coverage is genuine. flake8, mypy, black and isort are clean on the three touched files (the black diff on test_download.py is pre-existing with mock.patch(...) code this PR doesn't touch). CI is green across all 30 matrix jobs.

One finding worth fixing, inline on COARSE_MTIME_RECORD: the granularity=2.0 case is a silent no-op, so nothing in the suite actually pins the 2 s constant.

_populate_dandiset_yaml() makes the same assumption

download.py:588 is the same bug class in the same command, and it's the more interesting half of the story:

elif existing is DownloadExisting.SKIP or (
    existing is DownloadExisting.REFRESH
    and os.lstat(dandiset_yaml).st_mtime >= mtime.timestamp()
):

Same premise as the site being fixed — that an mtime we wrote with os.utime(dandiset_yaml, (time.time(), mtime.timestamp())) twelve lines below reads back exactly. On a coarse filesystem it reads back below the value we set, so the >= fails.

The symptom is quite different from #1907, though, and much milder — worth spelling out so the issue doesn't get closed on a wrong model of the blast radius:

1. No re-transfer churn. The yaml_load(fp, typ="safe") == metadata check above short-circuits with _skip_file("no change") whenever the content matches, which is the steady state. The mtime comparison is only reached when the metadata genuinely differs, and there redownloading is the correct outcome.
2. The one real loss is protection of local edits. That >= means "local copy is ahead of the record, leave it alone". Truncation makes a locally-modified dandiset.yaml look up to 2 s behind the record when it is in fact level with or slightly ahead of it, and ds.update_metadata(metadata) overwrites it. Needs the edit to land within the quantization window of dandiset.modified, so it's a corner — but it's silent data loss when it hits, whereas #1907 was merely slow.

Note the direction differs from the _download_file() site: this one is a one-sided >=, not a symmetric equality, so it wants st_mtime >= mtime.timestamp() - MTIME_TOLERANCE rather than an is_same_time() tolerance. One line, same constant, and it keeps the two round-trip assumptions in download.py from drifting apart. Either fold it in here, or leave a note on #1907 so it isn't marked fully addressed.

Minor, non-blocking

• Log readability. The new debug line prints raw epoch floats (local mtime: %f, record mtime: %f). Since sharper diagnostics is part of the point, ISO timestamps would serve someone reading a log far better than 1787412080.651000. Separately, %r applied to str(path) renders '/x/y' where the old f"{path!r}" rendered PosixPath('/x/y') — an improvement, just noting the format changed.
• Fixture hazard. quantizing_utime forwards times positionally, so a caller using os.utime(path, ns=...) would hit ValueError: you may specify either 'times' or 'ns' but not both. Nothing in dandi does that today; only a trap if the fixture gets reused.
• test_download_file_refresh_reports_mtime_mismatch regex-parses the debug message to recover the delta, coupling it to an incidental log format. Defensible since the diagnostics are part of this change, but asserting same == ["size"] more directly would be sturdier.

---

Generated by Claude Code

[CodyCBakerPhD]

The one real loss is protection of local edits. That >= means "local copy is ahead of the record, leave it alone". Truncation makes a locally-modified dandiset.yaml look up to 2 s behind the record when it is in fact level with or slightly ahead of it, and ds.update_metadata(metadata) overwrites it. Needs the edit to land within the quantization window of dandiset.modified, so it's a corner — but it's silent data loss when it hits, whereas #1907 was merely slow.

I'm not really concerned about this case TBH - we're talking about the CLI usage in particular here and that kind of time lag just isn't realistic (might be if we were to make some kind of interactive app that needed to constantly talk to the database though)

Addressed the other minor suggestions

[yarikoptic]

Re-reviewed the current head (32b7bab). The test constant, the ISO timestamps, the ns= fixture hardening and dropping the regex parse all look good — and thanks for catching that os.utime() only rejects times/ns when both are non-None, my ValueError claim was wrong.

Verified locally on this head: flake8/mypy/black/isort clean on the touched files, all four tests pass, and the constant is now genuinely pinned — MTIME_TOLERANCE = 1.0 fails coarse_mtime_fs[2.0], which it did not before. ensure_datetime(stat.st_mtime, tz=timezone.utc) also normalizes the same way is_same_time() does internally, so the two numbers in the log really are in the frame the comparison used.

One item from my first pass is still open — file comment on download.py for _populate_dandiset_yaml().

---

Generated by Claude Code

[CodyCBakerPhD]

One item from my first pass is still open — file comment on download.py for _populate_dandiset_yaml().

I did address that, actually, in my human response

Similar to my other comment #1910 (comment), if you really want it to include the incredibly small (and more likely to be updated at a regular basis on remote) dantiset.yaml file too I think you can do that in a follow-up. All I need to address MY human concerns here is what is on this PR

The fscacher question is an interesting one too but I haven't run any operations that felt 'slow' where it could make it feel 'fast'

[yarikoptic]

ok, please consider/review

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

[CodyCBakerPhD]

ok, please consider/review

#1912

I already did, 30 minutes earlier #1912 (comment)

[github-actions]

🚀 PR was released in 0.78.0 🚀