Overview
One interesting thing I've run into when working on #1900 is that OGGM doesn't seem to check if a cached file was truncated by an interrupted download. This leads to non-obvious errors because re-running a test or using reset=True doesn't refresh the download cache.
Expected Behaviour
A cache hit should only return for a file that is verified as complete. If the cached file is incomplete, it should re-download this.
Actual Behaviour
_cached_download_helper returns the cached path as soon as os.path.isfile(cache_path) is true, with no checks. Downloads are written directly to cache_path, and cleanup only removes a partial file if the download function itself raises an exception. Rerunning a command does nothing, so the file sticks around until the user clears the cache manually (so never). This affects any download (prepro bundles, DEMs, climate data, RGI files), and is more likely when multiprocessing is enabled and a worker is torn down mid-write.
Possible Solutions
- Atomic writes: download to
cache_path + '.tmp' then os.replace() into place once a transfer completes.
- Size validation on cache hit: check the expected size against
Content-Length, which is read in _requests_urlretrieve. I'm still mulling over how this will work with streaming.
- Optional integrity check: an opt-in checksum or
HEAD size check for stricter verification. Maybe this is fine for smaller files, but checksums for each gdir would just be a waste of compute time.
Maybe a mix of 1 and 2? I'll probably get back to this once I start work on streaming for #1896.
Overview
One interesting thing I've run into when working on #1900 is that OGGM doesn't seem to check if a cached file was truncated by an interrupted download. This leads to non-obvious errors because re-running a test or using
reset=Truedoesn't refresh the download cache.Expected Behaviour
A cache hit should only return for a file that is verified as complete. If the cached file is incomplete, it should re-download this.
Actual Behaviour
_cached_download_helperreturns the cached path as soon asos.path.isfile(cache_path)is true, with no checks. Downloads are written directly tocache_path, and cleanup only removes a partial file if the download function itself raises an exception. Rerunning a command does nothing, so the file sticks around until the user clears the cache manually (so never). This affects any download (prepro bundles, DEMs, climate data, RGI files), and is more likely when multiprocessing is enabled and a worker is torn down mid-write.Possible Solutions
cache_path + '.tmp'thenos.replace()into place once a transfer completes.Content-Length, which is read in_requests_urlretrieve. I'm still mulling over how this will work with streaming.HEADsize check for stricter verification. Maybe this is fine for smaller files, but checksums for each gdir would just be a waste of compute time.Maybe a mix of 1 and 2? I'll probably get back to this once I start work on streaming for #1896.