This is an automated email from the ASF dual-hosted git repository.
janhoy pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr-orbit.git
The following commit(s) were added to refs/heads/main by this push:
new c156d4d9 fix: prevent caching incomplete Solr downloads (#113)
c156d4d9 is described below
commit c156d4d9fdbf5d286824d589de76ef4ecdc95aee
Author: Mohamed Laghdaf Dadda <[email protected]>
AuthorDate: Fri Oct 16 15:37:55 2026 +0100
fix: prevent caching incomplete Solr downloads (#113)
* fix: prevent caching incomplete Solr downloads
Download Solr archives to temporary files in the cache directory
and atomically publish them only after successful retrieval.
Clean up temporary files on failure or interruption while preserving
mirror fallback behavior and propagating local filesystem errors.
Add regression tests for interrupted downloads, cache reuse,
mirror fallback, exhausted mirrors, and publication failures.
Fixes #104
* fix: preserve Solr download cache file permissions
---
solrorbit/builder/solr_provisioner.py | 22 +++--
tests/unit/solr/test_provisioner.py | 156 +++++++++++++++++++++++++++++++++-
2 files changed, 170 insertions(+), 8 deletions(-)
diff --git a/solrorbit/builder/solr_provisioner.py
b/solrorbit/builder/solr_provisioner.py
index 8d132fe4..a78cf226 100644
--- a/solrorbit/builder/solr_provisioner.py
+++ b/solrorbit/builder/solr_provisioner.py
@@ -28,6 +28,7 @@ import shutil
import socket
import subprocess
import tarfile
+import tempfile
import time
import urllib.request
from pathlib import Path
@@ -128,6 +129,7 @@ class SolrProvisioner:
"""
tarball = f"solr-{version}.tgz"
dest = os.path.join(self.cache_dir, tarball)
+
if os.path.exists(dest):
logger.info("Solr %s already cached at %s", version, dest)
return dest
@@ -136,14 +138,22 @@ class SolrProvisioner:
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)
- try:
- urllib.request.urlretrieve(url, dest)
+
+ with tempfile.TemporaryDirectory(
+ dir=self.cache_dir,
+ prefix=f"{tarball}."
+ ) as temp_dir:
+ temp_path = os.path.join(temp_dir, tarball)
+
+ 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)
logger.info("Downloaded Solr %s to %s", version, dest)
return dest
- except Exception as exc:
- logger.warning("Download from %s failed: %s", url, exc)
- if os.path.exists(dest):
- os.remove(dest)
raise SolrProvisionerError(
f"Could not download Solr {version} from any mirror. "
diff --git a/tests/unit/solr/test_provisioner.py
b/tests/unit/solr/test_provisioner.py
index 49801367..e02cf083 100644
--- a/tests/unit/solr/test_provisioner.py
+++ b/tests/unit/solr/test_provisioner.py
@@ -19,11 +19,13 @@
import socket
import unittest
+import os
+import tempfile
+import stat
from unittest.mock import MagicMock, patch
from solrorbit import exceptions
-from solrorbit.builder.solr_provisioner import SolrProvisioner,
SolrDockerLauncher, _assert_port_free
-
+from solrorbit.builder.solr_provisioner import SolrProvisioner,
SolrDockerLauncher, _assert_port_free, SolrProvisionerError
def _make_cluster_config(variables: dict):
"""Create a minimal cluster_config mock with the given variables dict."""
@@ -201,6 +203,156 @@ class TestAssertPortFree(unittest.TestCase):
with patch("subprocess.run", return_value=MagicMock(returncode=0)):
with self.assertRaises(exceptions.SystemSetupError):
launcher.start(version_tag="9")
+class TestProvisionerDownload(unittest.TestCase):
+ """Tests for Solr download caching and failure handling."""
+ def test_interrupted_download_does_not_cache_partial_file(self):
+
+ with tempfile.TemporaryDirectory() as cache_dir:
+ provisioner = SolrProvisioner(cache_dir=cache_dir)
+ dest = os.path.join(cache_dir, "solr-9.10.1.tgz")
+ self.assertFalse(os.path.exists(dest))
+
+ def fake_download(url, temp_path):
+ with open(temp_path, "wb") as file:
+ file.write(b"incomplete data")
+
+ self.assertFalse(os.path.exists(dest))
+
+ raise KeyboardInterrupt()
+
+ with self.assertRaises(KeyboardInterrupt):
+ with patch(
+
"solrorbit.builder.solr_provisioner.urllib.request.urlretrieve",
+ side_effect=fake_download
+ ):
+ provisioner.download("9.10.1")
+ self.assertFalse(os.path.exists(dest))
+ self.assertEqual([], os.listdir(cache_dir))
+
+ def test_successful_download_is_cached(self):
+ with tempfile.TemporaryDirectory() as cache_dir:
+ provisioner = SolrProvisioner(cache_dir=cache_dir)
+ dest = os.path.join(cache_dir, "solr-9.10.1.tgz")
+
+ def fake_download(url, temp_path):
+ with open(temp_path, "wb") as file:
+ file.write(b"complete data")
+
+ with patch(
+
"solrorbit.builder.solr_provisioner.urllib.request.urlretrieve",
+ side_effect=fake_download
+ ) as mock_download:
+ result = provisioner.download("9.10.1")
+ second_result = provisioner.download("9.10.1")
+ self.assertEqual(result, dest)
+ self.assertEqual(second_result, dest)
+ mock_download.assert_called_once()
+ self.assertTrue(os.path.exists(dest))
+ with open(dest, "rb") as file:
+ content = file.read()
+ self.assertEqual(content, b"complete data")
+
+ def test_mirror_fallback_after_partial_download(self):
+ with tempfile.TemporaryDirectory() as cache_dir:
+ provisioner = SolrProvisioner(cache_dir=cache_dir)
+ dest = os.path.join(cache_dir, "solr-9.10.1.tgz")
+ attempts = []
+
+ def fake_download(url, temp_path):
+ attempts.append(url)
+
+ if len(attempts) == 1:
+ with open(temp_path, "wb") as file:
+ file.write(b"partial data")
+
+ raise OSError("Simulated network failure")
+ else:
+ with open(temp_path, "wb") as file:
+ file.write(b"complete data")
+
+ with patch(
+
"solrorbit.builder.solr_provisioner.urllib.request.urlretrieve",
+ side_effect=fake_download
+ ):
+ result = provisioner.download("9.10.1")
+ self.assertEqual(result, dest)
+ self.assertEqual(len(attempts), 2)
+ self.assertTrue(os.path.exists(dest))
+ with open(dest, "rb") as file:
+ content = file.read()
+ self.assertEqual(content, b"complete data")
+ self.assertEqual([os.path.basename(dest)], os.listdir(cache_dir))
+
+ def test_all_mirrors_fail(self):
+ with tempfile.TemporaryDirectory() as cache_dir:
+ provisioner = SolrProvisioner(cache_dir=cache_dir)
+ dest = os.path.join(cache_dir, "solr-9.10.1.tgz")
+ attempts = []
+
+ def fake_download(url, temp_path):
+ attempts.append(url)
+
+ with open(temp_path, "wb") as file:
+ file.write(b"partial data")
+
+ raise OSError("Simulated network failure")
+
+ with patch(
+
"solrorbit.builder.solr_provisioner.urllib.request.urlretrieve",
+ side_effect=fake_download
+ ):
+ with self.assertRaises(SolrProvisionerError):
+ provisioner.download("9.10.1")
+
+ self.assertEqual(len(attempts), 3)
+
+ self.assertFalse(os.path.exists(dest))
+
+ self.assertEqual([], os.listdir(cache_dir))
+
+ def test_replace_failure_propagates(self):
+ with tempfile.TemporaryDirectory() as cache_dir:
+ provisioner = SolrProvisioner(cache_dir=cache_dir)
+
+ def fake_download(url, temp_path):
+ with open(temp_path, "wb") as file:
+ file.write(b"complete data")
+
+ with patch(
+
"solrorbit.builder.solr_provisioner.urllib.request.urlretrieve",
+ side_effect=fake_download
+ ):
+ with patch(
+ "solrorbit.builder.solr_provisioner.os.replace",
+ side_effect=PermissionError("Permission denied")
+ ):
+ with self.assertRaises(PermissionError):
+ provisioner.download("9.10.1")
+ dest = os.path.join(cache_dir, "solr-9.10.1.tgz")
+
+ self.assertFalse(os.path.exists(dest))
+
+ self.assertEqual([], os.listdir(cache_dir))
+
+ def test_download_preserves_file_permissions(self):
+ with tempfile.TemporaryDirectory() as cache_dir:
+ provisioner = SolrProvisioner(cache_dir=cache_dir)
+ reference_path = os.path.join(cache_dir, "reference.txt")
+ with open(reference_path, "wb") as file:
+ file.write(b"reference data")
+
+ def fake_download(url, temp_path):
+ with open(temp_path, "wb") as file:
+ file.write(b"complete data")
+
+ with patch(
+
"solrorbit.builder.solr_provisioner.urllib.request.urlretrieve",
+ side_effect=fake_download
+ ):
+ dest = provisioner.download("9.10.1")
+ expected_permissions =
stat.S_IMODE(os.stat(reference_path).st_mode)
+ actual_permissions = stat.S_IMODE(os.stat(dest).st_mode)
+ self.assertEqual(expected_permissions, actual_permissions)
if __name__ == "__main__":
