Repository navigation
Write downloads and cache entries atomically - #3692
kevinwwillis wants to merge 1 commit into
Conversation
BaseQuery._download_file wrote downloads directly to their final path, and to_cache wrote pickles in place. An interrupted transfer therefore left a truncated file under the final name. Callers that treat an existing file as complete (lightkurve checks only os.path.exists in its cache) then read it as a finished download, and when several users or processes share a download directory, one user's interrupted download breaks the file for everyone. Downloads now go to a unique temporary file next to the destination, are checked against the Content-Length of the response that delivered them, are fsync'ed and renamed into place. Data from an interrupted transfer are kept as <filename>.part and resumed with continuation=True (by copying, so the partial download and the destination stay usable by other processes); a 206 response is checked against the requested range. The destination keeps its previous content until the new file is complete. Cache pickles are written the same way, and unreadable cache entries are treated as a cache miss. MAST cutout zip files get unique names and are extracted atomically, since lightkurve finds TESScut cutouts by file name. ALMA's verify_only reports partial downloads kept as .part. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Hi 👋 and thank you for your contribution! 🙏 This pull request is being closed automatically - but don't worry, this is not the end, and we may re-open it. For some background, we have recently started seeing a rapid increase in the number of pull requests opened. Some of these are from good-faith humans, but some of which are from autonomous LLM agents or humans using LLMs who do not have a genuine understanding of, or interest in, the project. We are therefore auto-closing pull requests from new contributors, but if you are a good-faith human, we want to make sure your pull request gets considered! So if you would like us to re-open your pull request so that it gets reviewed, you need to do two things:
The second step is important, as we may otherwise miss notifications about this pull request. To be clear, our AI policy does allow the use of LLMs as part of contributions, but we need to see authentic engagement and understanding from humans making the contribution. Notes for maintainersIf you know this is a genuine user and the PR is is compliance with the AI policy, you may re-open this straight away. After two successful merged pull requests, the user will be added to the allowlist and future PRs will no longer be closed automatically. This user has not made any pull requests to this repository prior to this one. In addition, here are some statistics on the user's activity on GitHub:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3692 +/- ##
==========================================
+ Coverage 73.86% 73.95% +0.09%
==========================================
Files 228 228
Lines 21475 21577 +102
==========================================
+ Hits 15863 15958 +95
- Misses 5612 5619 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
BaseQuery._download_filewrites a download directly to its final path, andto_cachewrites the query-result pickle in place. If a transfer is interrupted (dropped connection, Ctrl-C, a killed kernel), a truncated file is left under the final name.Several callers treat an existing file as a complete download. lightkurve checks only
os.path.exists(<cache>/mastDownload/<collection>/<obs_id>/<file>)before skippingObservations.download_products, and it finds TESScut cutouts by file-name glob. So one interrupted download leaves a broken file that is read as finished from then on. This is worse when a download directory is shared between users or processes (for example a lab-wide lightkurve cache on a JupyterHub server): one user's interrupted download breaks the file for everyone.Change
BaseQuery._download_file):<file>.<8 hex>.part).Content-Lengthof the response that delivered the data. This is skipped when the body was compressed in transit.fsync'ed and then renamed into place withos.replace. On Windows the rename is retried briefly. If the destination is in use but already holds the same complete file, the call succeeds.<file>.part, and a later call withcontinuation=Trueresumes from them. An incomplete destination file left by an older astroquery is resumed the same way..partmissing..partis only replaced by one with more data, and it is removed after a successful download.Rangeheader is now always removed from the session. It used to stay set if the request raised.Pathdestinations work, as MAST passesPathobjects. A symlink at the destination is followed, as writing through it did before.to_cache):from_cachetreats an unreadable (torn) pickle as a cache miss.clear_cachealso removes leftover temporary entries.ZipFile.extract, which sanitises member names, and each file is renamed into place. A cutout file is therefore never seen partly written.verify_only: it reports partial downloads that are now kept as.part.Known limitation
A process killed outright (SIGKILL, OOM, SIGHUP without a Python exception) does not reach the cleanup, so its data stay in the uniquely named temporary file instead of
<file>.part. Such a file is not resumed and has to be cleaned up separately.This is still strictly better than before, when the final path itself held the partial data. Making this case resumable would need per-file advisory locking (
fcntl.flock/msvcrt.locking). I left that out to keep this change platform-neutral, but I'm happy to add it if maintainers prefer.Tests
New tests in
astroquery/tests/test_query.py,test_cache.pyandmast/tests/test_mast.pycover:strandPathdestinations, and checks that the destination is absent or unchanged while the transfer runs;Rangeor answers with a different range;.partfiles;verbose=Falsewithcache=True;to_cache, a torn cache entry, andclear_cache;The new download and cache tests fail against the current
main. The mocked response inpatch_getnow sets its range status and headers when they are applied, as a server would.Offline results:
pytest astroquery: 2181 passed, 2679 skipped, 2 xfailed (Windows 11, Python 3.11).astroquery/tests,astroquery/mast/testsandastroquery/alma/tests: 242 passed, 129 skipped (Linux, Python 3.11).Context: we run a shared lightkurve download cache for a research group's JupyterHub, and this was the one thing that made it unsafe.
🤖 Generated with Claude Code