From 16667ef26091e0a42bd641d5c228bb4cec5b62a3 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 8 Jul 2024 16:32:54 +0200 Subject: [PATCH] sources(curl): error if curl exists 0 but there are downloads left As part of the investigation of the CI failure in https://github.com/osbuild/osbuild-composer/pull/4247 we noticed that curl can return a return_code of `0` even when it did not downloaded all the urls in a `--config` provided file. This seems to be curl version dependent, I had a hard time writing a test-case with the real curl (8.6.0) that reproduces this so I went with mocking it. We definietly saw this failure with the centos 9 version (7.76). Our current code is buggy and assumes that the exit status of curl is always non-zero if any download fails but that is only the case when `--fail-early` is used. The extra paranoia will not hurt even when relying on the exit code of curl is fixed. --- sources/org.osbuild.curl | 3 +++ sources/test/test_curl_source.py | 14 ++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/sources/org.osbuild.curl b/sources/org.osbuild.curl index db271ff3..c858ff61 100755 --- a/sources/org.osbuild.curl +++ b/sources/org.osbuild.curl @@ -317,6 +317,9 @@ class CurlSource(sources.SourceService): failed_urls = ",".join([itm[1]["url"] for itm in dl_pairs]) raise RuntimeError(f"curl: error downloading {failed_urls}: error code {return_code}") + if len(dl_pairs) > 0: + raise RuntimeError(f"curl: finished with return_code {return_code} but {dl_pairs} left to download") + def _fetch_all_old_curl(self, amended): with concurrent.futures.ThreadPoolExecutor(max_workers=self.max_workers) as executor: for _ in executor.map(self.fetch_one, *zip(*amended)): diff --git a/sources/test/test_curl_source.py b/sources/test/test_curl_source.py index a3fd45f0..5a591a9e 100644 --- a/sources/test/test_curl_source.py +++ b/sources/test/test_curl_source.py @@ -9,6 +9,7 @@ from unittest.mock import patch import pytest +import osbuild.testutil from osbuild.testutil.net import http_serve_directory SOURCES_NAME = "org.osbuild.curl" @@ -306,3 +307,16 @@ def test_curl_has_parallel_download(mocked_check_output, monkeypatch, sources_mo mocked_check_output.return_value = OLD_CURL_OUTPUT assert not sources_module.curl_has_parallel_downloads() + + +# this check is only done in the "parallel=True" case +@pytest.mark.parametrize("curl_parallel", [True], indirect=["curl_parallel"]) +def test_curl_result_is_double_checked(tmp_path, curl_parallel): + test_sources = make_test_sources(tmp_path, 1234, 5) + + # simulate that curl returned an exit code 0 even though not all + # sources got downloaded + with osbuild.testutil.mock_command("curl", ""): + with pytest.raises(RuntimeError) as exp: + curl_parallel.fetch_all(test_sources) + assert re.match(r"curl: finished with return_code 0 but .* left to download", str(exp.value))