You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "med9110 (via GitHub)" <gi...@apache.org> on 2026/10/15 18:23:16 UTC
[PR] fix: prevent caching incomplete Solr downloads [solr-orbit]
med9110 opened a new pull request, #113:
URL: https://github.com/apache/solr-orbit/pull/113
### Description
Previously, `SolrProvisioner.download()` wrote directly to the final cache path. If a download was interrupted, it could leave a partial `.tgz` file that subsequent runs would incorrectly treat as a valid cached archive.
This change prevents incomplete downloads from being cached by:
- Downloading archives to temporary files in the cache directory and atomically publishing them with `os.replace()` only after successful retrieval.
- Cleaning up temporary files when downloads fail or are interrupted.
- Preserving mirror fallback behavior for download failures while allowing local filesystem errors to propagate.
### Issues Resolved
Fixes #104
### Testing
- [x] New functionality includes testing
Added five regression tests covering:
- Interrupted downloads do not leave partial cached archives.
- Successful downloads are cached and reused.
- Mirror fallback works after a partial download failure.
- Exhausting all mirrors raises `SolrProvisionerError` without leaving incomplete files.
- Publication failures propagate correctly and clean up temporary files.
**Validation:**
- `uv run pytest tests/ -q` — 1,145 passed, 5 skipped.
- `uv run ruff check .` — All checks passed.
- `git diff --check` — No whitespace errors.
---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] fix: prevent caching incomplete Solr downloads [solr-orbit]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy merged PR #113:
URL: https://github.com/apache/solr-orbit/pull/113
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] fix: prevent caching incomplete Solr downloads [solr-orbit]
Posted by "med9110 (via GitHub)" <gi...@apache.org>.
med9110 commented on code in PR #113:
URL: https://github.com/apache/solr-orbit/pull/113#discussion_r4293870662
##########
solrorbit/builder/solr_provisioner.py:
##########
@@ -136,14 +138,29 @@ def download(self, version: str) -> str:
for base_url in (_DOWNLOADS_URL, _ARCHIVE_9_URL, _ARCHIVE_PRE9_URL):
url = f"{base_url}/{version}/{tarball}"
logger.info("Downloading Solr %s from %s", version, url)
+
+ temp_path = None
+
try:
- urllib.request.urlretrieve(url, dest)
+ fd, temp_path = tempfile.mkstemp(
+ dir=self.cache_dir,
+ prefix=f"{tarball}.",
+ suffix=".tmp"
+ )
+ os.close(fd)
+ try:
+ urllib.request.urlretrieve(url, temp_path)
+ except Exception as exc:
+ logger.warning("Download from %s failed: %s", url, exc)
+ continue
+ os.replace(temp_path, dest)
Review Comment:
Thanks @janhoy, good catch!
I've addressed this in `bb2e522c` by switching from `mkstemp()` to `TemporaryDirectory()`, allowing `urlretrieve()` to create the archive with permissions respecting the system's `umask`, rather than forcing `0600`.
The download still uses `os.replace()` for atomic publication, and the temporary directory is cleaned up on success or failure.
I also added a permissions regression test and strengthened the cleanup assertions.
Verified locally on Python 3.13: **1,146 passed, 5 skipped**, and Ruff checks passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] fix: prevent caching incomplete Solr downloads [solr-orbit]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #113:
URL: https://github.com/apache/solr-orbit/pull/113#discussion_r4287102646
##########
solrorbit/builder/solr_provisioner.py:
##########
@@ -136,14 +138,29 @@ def download(self, version: str) -> str:
for base_url in (_DOWNLOADS_URL, _ARCHIVE_9_URL, _ARCHIVE_PRE9_URL):
url = f"{base_url}/{version}/{tarball}"
logger.info("Downloading Solr %s from %s", version, url)
+
+ temp_path = None
+
try:
- urllib.request.urlretrieve(url, dest)
+ fd, temp_path = tempfile.mkstemp(
+ dir=self.cache_dir,
+ prefix=f"{tarball}.",
+ suffix=".tmp"
+ )
+ os.close(fd)
+ try:
+ urllib.request.urlretrieve(url, temp_path)
+ except Exception as exc:
+ logger.warning("Download from %s failed: %s", url, exc)
+ continue
+ os.replace(temp_path, dest)
Review Comment:
`tempfile.mkstemp()` creates its file with mode `0600`, whereas the old `urlretrieve(url, dest)` wrote through a plain `open(dest, 'wb')` and so got `0o666 & ~umask` — `0644` under the usual `umask 022`. Because `os.replace()` preserves the source file's mode, this change silently makes every cached tarball owner-only.
That's harmless for a per-user `~/.solr-orbit/cache`, but it breaks a cache directory shared between users, or one written by a CI job and later read by a different account.
An explicit chmod before publishing preserves the previous behaviour:
```python
umask = os.umask(0)
os.umask(umask)
os.chmod(temp_path, 0o666 & ~umask)
os.replace(temp_path, dest)
```
`0o666 & ~umask` is what `open()` would have applied itself. Reading the umask requires that set-and-restore dance because there is no getter, and it is not thread-safe, so if you would rather not touch the process umask then a plain `os.chmod(temp_path, 0o644)` is a reasonable simpler alternative.
🤖 Assisted by Claude Code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org