Skip to content

Write downloads and cache entries atomically - #3692

Closed
kevinwwillis wants to merge 1 commit into
astropy:mainfrom
kevinwwillis:atomic-downloads
Closed

kevinwwillis wants to merge 1 commit into
astropy:mainfrom
kevinwwillis:atomic-downloads

Conversation

@kevinwwillis

Copy link
Copy Markdown

Problem

BaseQuery._download_file writes a download directly to its final path, and to_cache writes 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 skipping Observations.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

  • Downloads (BaseQuery._download_file):
    • Data are written to a unique temporary file next to the destination (<file>.<8 hex>.part).
    • The size is checked against the Content-Length of the response that delivered the data. This is skipped when the body was compressed in transit.
    • The file is fsync'ed and then renamed into place with os.replace. On Windows the rename is retried briefly. If the destination is in use but already holds the same complete file, the call succeeds.
    • The destination keeps its previous content until the new file is complete.
  • Partial downloads:
    • Data from an interrupted transfer are kept as <file>.part, and a later call with continuation=True resumes from them. An incomplete destination file left by an older astroquery is resumed the same way.
    • Resuming copies the partial data rather than moving them, so concurrent processes never find the destination or the .part missing.
    • A 206 response must cover the requested range. Otherwise the whole file is downloaded.
    • A .part is only replaced by one with more data, and it is removed after a successful download.
    • The Range header is now always removed from the session. It used to stay set if the request raised.
  • Path destinations: Path destinations work, as MAST passes Path objects. A symlink at the destination is followed, as writing through it did before.
  • Cache entries (to_cache):
    • Entries are written to a temporary file and renamed into place.
    • A failed rename (an entry in use on Windows) logs a warning and keeps the existing entry.
    • from_cache treats an unreadable (torn) pickle as a cache miss.
    • clear_cache also removes leftover temporary entries.
  • MAST cutouts:
    • Zip files get unique names; they used to be unique only to the second, so two requests in the same second collided.
    • Zips are extracted into a temporary directory with ZipFile.extract, which sanitises member names, and each file is renamed into place. A cutout file is therefore never seen partly written.
  • ALMA 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.py and mast/tests/test_mast.py cover:

  • an interrupted transfer and its resume, with str and Path destinations, and checks that the destination is absent or unchanged while the transfer runs;
  • a short body without a transport error;
  • a server that ignores Range or answers with a different range;
  • a HEAD length that differs from the GET length;
  • empty, complete, oversize and stale .part files;
  • a legacy partial destination;
  • a failed final rename, and a destination in use but complete;
  • an interrupted resume copy;
  • verbose=False with cache=True;
  • a symlink destination;
  • atomic to_cache, a torn cache entry, and clear_cache;
  • atomic and sanitised cutout extraction, and unique zip names.

The new download and cache tests fail against the current main. The mocked response in patch_get now 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/tests and astroquery/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

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>
@giles

giles Bot commented Oct 22, 2026

Copy link
Copy Markdown

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:

  1. Add a comment here to explain why you need the changes here to be considered, as in how the bug or missing feature affects your work, or whether this is an issue you have encountered but does not affect you.

  2. Join the astropy slack using this invite link and head over to the #new-contributor-requests channel to introduce yourself and let us know about this pull request

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 maintainers

If 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:

Last 24 hours Last 7 days
Pull requests opened 5 5
Issues opened 0 2

@giles giles Bot closed this Oct 22, 2026
@codecov

codecov Bot commented Oct 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.95%. Comparing base (da1bfdd) to head (8418f3e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
astroquery/query.py 91.54% 12 Missing ⚠️
astroquery/alma/core.py 0.00% 7 Missing ⚠️
astroquery/mast/cutouts.py 88.88% 3 Missing ⚠️
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.
📢 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant