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

[yarikoptic]

_populate_dandiset_yaml(), at line 594, makes the same round-trip assumption this PR is removing from _download_file() — and it's left untouched. (File-level comment because the line is outside this PR's diff hunks, so GitHub won't take an applicable suggestion there.)

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

That local mtime is also one we set ourselves, twelve lines below:

os.utime(dandiset_yaml, (time.time(), mtime.timestamp()))

so on a quantizing destination it reads back below what we wrote, and the >= fails.

Suggested (verified black and flake8 clean; MTIME_TOLERANCE is already imported in this module):

        elif existing is DownloadExisting.SKIP or (
            existing is DownloadExisting.REFRESH
            # This mtime is also one we set ourselves with os.utime() below, so
            # the same filesystem round trip applies here as in
            # _download_file(); without the tolerance a destination that
            # quantizes mtimes makes the local copy look older than the record
            # it was set from.  See https://github.com/dandi/dandi-cli/issues/1907
            and os.lstat(dandiset_yaml).st_mtime >= mtime.timestamp() - MTIME_TOLERANCE
        ):

Note the direction differs from the _download_file() site — this is a one-sided >=, not a symmetric equality, so it wants the constant subtracted rather than an is_same_time() tolerance.

The symptom is also quite different from #1907, and worth being precise about so the issue doesn't get closed on the wrong model of the blast radius:

1. No re-transfer churn here. 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. This comparison is only reached when the metadata genuinely differs — and there, redownloading is the correct outcome.
2. What's actually lost is protection of local edits. That >= means "the 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 ahead of it, and ds.update_metadata(metadata) then overwrites it. It needs the edit to land within the quantization window of dandiset.modified, so it is a corner — but it is silent data loss when it hits, where download -e refresh never skips anything on filesystems without sub-second mtimes #1907 was merely slow.

Fine by me to defer this to a follow-up rather than widen the PR — but then #1907 shouldn't be closed as fully addressed, since the same faulty premise survives in the same command.

---

Generated by Claude Code

[yarikoptic]

initially I thought that we must go instead the way "you" approached in

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

instead since this property is really file system dependent and not per se a global const... But there are indeed tolerances to observe but also I do not want to loosen it up needlessly a lot on systems where there is sufficient mtime records precision... thinking also about the fact that if precision is not there -- it is rounded to nearest number not just "noise" in the time value thus giving us an alternative and somewhat more appropriate decision process to consider it "the same".

I am preparing a potential PR to this PR to see if we could refine that way

[yarikoptic]

also this is a great example for where to start using https://github.com/con/eval-under, sweeping over filesystems, at least for some portion of the likely related tests (download/uploads)... here fires up one more claude code session to formalize the action to (re)use ;-)

[yarikoptic]

also now I wonder how it would affect fscacher used intensively here in dandi-cli , also unrelated to this changes, just identifying other affected blocks which might tune up to work "as expected" on such wonderful file systems

[CodyCBakerPhD]

Why would we need nanosecond resolution when supporting this feature on any filesystem? I'm thinking about the actual human use of the affected behavior (downloading a dataset)

My point to favoring this approach was simplicity, which yes means a 'global solution' but also does not mean overfitting to any one system

So I do feel like a lot of this is just 'overthinking it'

[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)