From 256c468f384084d2559b41eece05c04a914c1957 Mon Sep 17 00:00:00 2001 From: Daksha1611 Date: Sat, 12 Sep 2026 03:03:04 +0530 Subject: [PATCH 1/3] Do not divide by zero when a download reports no Content-Length print_progress() computed (float)count / max with max taken straight from libcurl's dltotal. A response using chunked transfer encoding reports dltotal == 0, and the guard in progress_callback only short-circuits while dltotal == dlnow, so as soon as any bytes arrive the callback falls through to print_progress(dlnow, 0, ...). The ratio is then infinite. Converting that to int is undefined; on x86-64 it yields INT_MIN, so the bar-fill loop does not run and the padding loop below it runs from INT_MIN to bar_width - roughly 2.1 billion putchar calls on every progress tick, once a second. The pull looks frozen and floods the terminal. Handle an unknown total explicitly by reporting the running byte count instead of a percentage, and move the bar arithmetic into computeProgressBarCells(), which returns 0 for an unknown total and clamps the result to [0, barWidth]. The clamp also covers a server reporting more bytes than it announced, which previously overran the bar. computeProgressBarCells() is declared in curl_downloader.hpp only so the arithmetic can be unit tested - print_progress() itself is a file-local static that writes to stdout. Happy to inline it and drop the tests if the smaller header surface is preferred. Tests: adds CurlDownloaderProgressTest covering the unknown-total case that caused the hang, normal ratio tracking, and clamping at both ends. --- src/pull_module/curl_downloader.cpp | 34 ++++++++++++++++++++++++++++- src/pull_module/curl_downloader.hpp | 6 +++++ src/test/pull_hf_model_test.cpp | 25 +++++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/pull_module/curl_downloader.cpp b/src/pull_module/curl_downloader.cpp index 5c0243550b..ad6f53e18d 100644 --- a/src/pull_module/curl_downloader.cpp +++ b/src/pull_module/curl_downloader.cpp @@ -46,13 +46,45 @@ static void print_download_speed_info(size_t received_size, size_t elapsed_time) printf(" [%.2f %s/s] ", rate, sizeUnits[rate_unit_idx]); } +int computeProgressBarCells(size_t count, size_t max, int barWidth) { + if (max == 0 || barWidth <= 0) { + return 0; + } + const double ratio = static_cast(count) / static_cast(max); + // Written as a positive test so a NaN ratio also lands here rather than falling through. + if (!(ratio > 0.0)) { + return 0; + } + if (ratio >= 1.0) { + return barWidth; + } + return static_cast(ratio * barWidth); +} + static void print_progress(size_t count, size_t max, bool first_run, size_t elapsed_time) { + // A response with no Content-Length reports dltotal == 0, so there is no ratio to show; + // report the running byte count instead. Dividing by max here yielded an infinite ratio + // whose conversion to int is undefined - in practice INT_MIN, which drove the padding + // loop below through roughly 2.1 billion putchar calls on every progress tick. + if (max == 0) { + double received = (double)count; + size_t receivedUnitId = 0; + while (received > 1000 && sizeUnits[receivedUnitId + 1]) { + received /= 1000.0; + receivedUnitId++; + } + printf("\rProgress: %.2f %s downloaded, total size unknown", received, sizeUnits[receivedUnitId]); + print_download_speed_info(count, elapsed_time); + fflush(stdout); + return; + } + float progress = (float)count / max; if (!first_run && progress < 0.01 && count > 0) return; const int bar_width = 50; - int bar_length = progress * bar_width; + const int bar_length = computeProgressBarCells(count, max, bar_width); printf("\rProgress: ["); int i; diff --git a/src/pull_module/curl_downloader.hpp b/src/pull_module/curl_downloader.hpp index 12a9ab39c5..386757a20d 100644 --- a/src/pull_module/curl_downloader.hpp +++ b/src/pull_module/curl_downloader.hpp @@ -14,6 +14,7 @@ // See the License for the specific language governing permissions and // limitations under the License. //***************************************************************************** +#include #include namespace ovms { @@ -23,4 +24,9 @@ Status downloadFileWithCurl(const std::string& url, const std::string& filePath) Status downloadFileWithCurl(const std::string& url, const std::string& filePath, const std::string& authTokenHF); Status fetchUrlToString(const std::string& url, const std::string& authToken, std::string& responseBody); +// Number of filled cells in a barWidth-wide progress bar for count out of max bytes, +// clamped to [0, barWidth]. max == 0 means the server sent no Content-Length, so there is +// no ratio to render and the result is 0. Declared here so the arithmetic can be unit tested. +int computeProgressBarCells(size_t count, size_t max, int barWidth); + } // namespace ovms diff --git a/src/test/pull_hf_model_test.cpp b/src/test/pull_hf_model_test.cpp index 5995159f78..9ba9170b61 100644 --- a/src/test/pull_hf_model_test.cpp +++ b/src/test/pull_hf_model_test.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -47,6 +48,7 @@ #include "src/test/test_file_utils.hpp" #include "src/test/test_with_temp_dir.hpp" #include "src/filesystem/filesystem.hpp" +#include "src/pull_module/curl_downloader.hpp" #include "src/pull_module/hf_pull_model_module.hpp" #include "src/pull_module/libgit2.hpp" #include "src/pull_module/optimum_export.hpp" @@ -378,6 +380,29 @@ ::testing::AssertionResult interruptPosixWorkerAndExpectGracefulExit(pid_t child } // namespace +// A response without Content-Length makes libcurl report dltotal == 0. The progress bar must +// not divide by it: the ratio becomes infinite and converting that to int is undefined, which +// in practice produced INT_MIN and a ~2.1 billion iteration padding loop. +TEST(CurlDownloaderProgressTest, UnknownTotalYieldsNoFilledCells) { + EXPECT_EQ(ovms::computeProgressBarCells(0, 0, 50), 0); + EXPECT_EQ(ovms::computeProgressBarCells(1024, 0, 50), 0); + EXPECT_EQ(ovms::computeProgressBarCells(std::numeric_limits::max(), 0, 50), 0); +} + +TEST(CurlDownloaderProgressTest, FilledCellsTrackRatio) { + EXPECT_EQ(ovms::computeProgressBarCells(0, 100, 50), 0); + EXPECT_EQ(ovms::computeProgressBarCells(50, 100, 50), 25); + EXPECT_EQ(ovms::computeProgressBarCells(100, 100, 50), 50); +} + +// Some servers report more bytes transferred than announced; the bar must stay within its width +// so the padding loop below it always runs a sane number of times. +TEST(CurlDownloaderProgressTest, FilledCellsClampToBarWidth) { + EXPECT_EQ(ovms::computeProgressBarCells(200, 100, 50), 50); + EXPECT_EQ(ovms::computeProgressBarCells(100, 100, 0), 0); + EXPECT_EQ(ovms::computeProgressBarCells(100, 100, -1), 0); +} + // RAII helper class for managing log file lifecycle. // Creates a log file path and automatically removes it on destruction. class LogFileGuard { From e31e15f460eee83923c495ed2885e9fdde8481fa Mon Sep 17 00:00:00 2001 From: Rafal Sapala Date: Mon, 14 Sep 2026 11:29:02 +0200 Subject: [PATCH 2/3] Add tets and fix patch --- src/test/pull_hf_model_test.cpp | 48 +++++++++++++++++++++++++++++++++ third_party/libgit2/lfs.patch | 5 ++-- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/src/test/pull_hf_model_test.cpp b/src/test/pull_hf_model_test.cpp index 9ba9170b61..28071a8eb6 100644 --- a/src/test/pull_hf_model_test.cpp +++ b/src/test/pull_hf_model_test.cpp @@ -13,10 +13,12 @@ // See the License for the specific language governing permissions and // limitations under the License. //***************************************************************************** +#include #include #include #include #include +#include #include #include #include @@ -403,6 +405,52 @@ TEST(CurlDownloaderProgressTest, FilledCellsClampToBarWidth) { EXPECT_EQ(ovms::computeProgressBarCells(100, 100, -1), 0); } +// Regression test for the call-site, not just the extracted helper: a real chunked-transfer +// response (no Content-Length) drives libcurl's progress callback with dltotal == 0 on every +// tick. Before the fix this hung the download in a ~2.1 billion iteration padding loop; here +// we bound the wait so a reintroduced regression fails instead of hanging the test suite. +TEST_F(TestWithTempDir, ChunkedTransferWithoutContentLengthDoesNotHang) { + const std::string body(64 * 1024, 'x'); + httplib::Server server; + server.Get("/chunked", [&body](const httplib::Request&, httplib::Response& res) { + res.set_chunked_content_provider("application/octet-stream", + [&body](size_t offset, httplib::DataSink& sink) { + if (offset >= body.size()) { + sink.done(); + return true; + } + const size_t chunkSize = std::min(4096, body.size() - offset); + sink.write(body.data() + offset, chunkSize); + return true; + }); + }); + const int port = server.bind_to_any_port("127.0.0.1"); + ASSERT_GT(port, 0); + std::thread serverThread([&server]() { + server.listen_after_bind(); + }); + server.wait_until_ready(); + + const std::string url = "http://127.0.0.1:" + std::to_string(port) + "/chunked"; + const std::string outputPath = directoryPath + "/downloaded.bin"; + + auto downloadFuture = std::async(std::launch::async, [&url, &outputPath]() { + return ovms::downloadFileWithCurl(url, outputPath); + }); + + ASSERT_EQ(downloadFuture.wait_for(std::chrono::seconds(10)), std::future_status::ready) + << "downloadFileWithCurl did not return in time for a chunked, Content-Length-less response"; + EXPECT_EQ(downloadFuture.get(), ovms::StatusCode::OK); + + server.stop(); + serverThread.join(); + + std::ifstream downloadedFile(outputPath, std::ios::binary); + std::ostringstream downloadedContent; + downloadedContent << downloadedFile.rdbuf(); + EXPECT_EQ(downloadedContent.str(), body); +} + // RAII helper class for managing log file lifecycle. // Creates a log file path and automatically removes it on destruction. class LogFileGuard { diff --git a/third_party/libgit2/lfs.patch b/third_party/libgit2/lfs.patch index 887d700201..961f4f83a3 100644 --- a/third_party/libgit2/lfs.patch +++ b/third_party/libgit2/lfs.patch @@ -427,7 +427,7 @@ new file mode 100644 index 000000000..18490e5ad --- /dev/null +++ b/src/libgit2/lfs_filter.c -@@ -0,0 +1,2014 @@ +@@ -0,0 +1,2015 @@ +/* +/ Copyright 2025 Intel Corporation +/ @@ -1565,7 +1565,8 @@ index 000000000..18490e5ad + return; + + bar_width = 50; -+ bar_length = progress * bar_width; ++ /* Clamp: a server reporting more bytes than dltotal must not overflow bar_width. */ ++ bar_length = (progress >= 1.0) ? bar_width : (int)(progress * bar_width); + + printf("\rProgress: ["); + for (i = 0; i < bar_length; ++i) { From 14d3f3c717b3d82d5be30d424093fec687650a55 Mon Sep 17 00:00:00 2001 From: Rafal Sapala Date: Mon, 14 Sep 2026 15:21:45 +0200 Subject: [PATCH 3/3] Fix segfault on error --- src/pull_module/curl_downloader.cpp | 35 ++++++++++++++++++++++++----- src/test/pull_hf_model_test.cpp | 13 ++++++----- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/pull_module/curl_downloader.cpp b/src/pull_module/curl_downloader.cpp index ad6f53e18d..1ac6c5172f 100644 --- a/src/pull_module/curl_downloader.cpp +++ b/src/pull_module/curl_downloader.cpp @@ -15,10 +15,12 @@ //***************************************************************************** #include "curl_downloader.hpp" +#include #include #include #include #include +#include #include #include @@ -149,6 +151,25 @@ static size_t file_write_callback(void* buffer, size_t size, size_t nmemb, void* } \ } while (0) +// libcurl requires curl_global_init/curl_global_cleanup to run exactly once per process; +// calling curl_global_cleanup() after every download tears down global TLS/engine state +// still needed elsewhere (other in-flight curl users), which segfaults on next use. +static Status ensureCurlGlobalInit() { + static std::once_flag initFlag; + static CURLcode initResult = CURLE_OK; + std::call_once(initFlag, []() { + initResult = curl_global_init(CURL_GLOBAL_DEFAULT); + if (initResult == CURLE_OK) { + std::atexit([]() { curl_global_cleanup(); }); + } + }); + if (initResult != CURLE_OK) { + SPDLOG_ERROR("curl error: {}. Error code: {}", curl_easy_strerror(initResult), (int)initResult); + return StatusCode::INTERNAL_ERROR; + } + return StatusCode::OK; +} + struct ProgressData { time_t started_download; time_t last_print_time; @@ -191,9 +212,10 @@ Status downloadFileWithCurl(const std::string& url, const std::string& filePath, std::string agentString = std::string(PROJECT_NAME) + "/" + std::string(PROJECT_VERSION); CURL* curl = nullptr; - CHECK_CURL_CALL(curl_global_init(CURL_GLOBAL_DEFAULT)); - auto globalCurlGuard = std::unique_ptr( - nullptr, [](void*) { curl_global_cleanup(); }); + auto initStatus = ensureCurlGlobalInit(); + if (!initStatus.ok()) { + return initStatus; + } curl = curl_easy_init(); if (!curl) { SPDLOG_ERROR("Failed to initialize cURL."); @@ -243,9 +265,10 @@ Status fetchUrlToString(const std::string& url, const std::string& authToken, st std::string agentString = std::string(PROJECT_NAME) + "/" + std::string(PROJECT_VERSION); CURL* curl = nullptr; - CHECK_CURL_CALL(curl_global_init(CURL_GLOBAL_DEFAULT)); - auto globalCurlGuard = std::unique_ptr( - nullptr, [](void*) { curl_global_cleanup(); }); + auto initStatus = ensureCurlGlobalInit(); + if (!initStatus.ok()) { + return initStatus; + } curl = curl_easy_init(); if (!curl) { SPDLOG_ERROR("Failed to initialize cURL."); diff --git a/src/test/pull_hf_model_test.cpp b/src/test/pull_hf_model_test.cpp index 28071a8eb6..e570ea3b9d 100644 --- a/src/test/pull_hf_model_test.cpp +++ b/src/test/pull_hf_model_test.cpp @@ -2526,7 +2526,8 @@ TEST_F(HfPullModelModuleLoraTest, ResolveHfLoraFilenames) { ovms::ImageGenerationGraphSettingsImpl graphSettings; ovms::LoraAdapterSettings adapter; adapter.alias = "pokemon"; - adapter.sourceLora = "juliensimon/sd-pokemon-lora"; + // juliensimon/sd-pokemon-lora was removed upstream; replaced with a repo verified to still exist. + adapter.sourceLora = "MohamedAhmedAE/stable-diffusion-v1-5_lora_finetuning"; adapter.sourceType = ovms::LoraSourceType::HF_REPO; graphSettings.loraAdapters.push_back(adapter); settings.graphSettings = graphSettings; @@ -2553,7 +2554,8 @@ TEST_F(HfPullModelModuleLoraTest, PullLoraAdaptersFromHfRepo) { ovms::ImageGenerationGraphSettingsImpl graphSettings; ovms::LoraAdapterSettings adapter; adapter.alias = "pokemon"; - adapter.sourceLora = "juliensimon/sd-pokemon-lora"; + // juliensimon/sd-pokemon-lora was removed upstream; replaced with a repo verified to still exist. + adapter.sourceLora = "MohamedAhmedAE/stable-diffusion-v1-5_lora_finetuning"; adapter.safetensorsFile = "pytorch_lora_weights.safetensors"; // explicit filename — skips HF API resolve adapter.sourceType = ovms::LoraSourceType::HF_REPO; graphSettings.loraAdapters.push_back(adapter); @@ -2562,7 +2564,7 @@ TEST_F(HfPullModelModuleLoraTest, PullLoraAdaptersFromHfRepo) { auto status = module.testPullLoraAdapters(this->directoryPath); ASSERT_TRUE(status.ok()) << status.string(); - auto loraFilePath = ovms::FileSystem::joinPath({this->directoryPath, "loras", "juliensimon/sd-pokemon-lora", "pytorch_lora_weights.safetensors"}); + auto loraFilePath = ovms::FileSystem::joinPath({this->directoryPath, "loras", "MohamedAhmedAE/stable-diffusion-v1-5_lora_finetuning", "pytorch_lora_weights.safetensors"}); ASSERT_TRUE(std::filesystem::exists(loraFilePath)) << loraFilePath; EXPECT_GT(std::filesystem::file_size(loraFilePath), 0); } @@ -2612,7 +2614,8 @@ TEST_F(HfDownloaderPullHfModel, DownloadImageGenModelWithLoRA) { std::string modelName = "OpenVINO/stable-diffusion-v1-5-int8-ov"; std::string downloadPath = ovms::FileSystem::joinPath({this->directoryPath, "repository"}); std::string task = "image_generation"; - std::string sourceLoras = "pokemon=juliensimon/sd-pokemon-lora@pytorch_lora_weights.safetensors"; + // juliensimon/sd-pokemon-lora was removed upstream; replaced with a repo verified to still exist. + std::string sourceLoras = "pokemon=MohamedAhmedAE/stable-diffusion-v1-5_lora_finetuning@pytorch_lora_weights.safetensors"; ::SetUpServerForDownloadWithLoras(this->t, this->server, modelName, downloadPath, task, sourceLoras); std::string basePath = ovms::FileSystem::joinPath({downloadPath, "OpenVINO", "stable-diffusion-v1-5-int8-ov"}); @@ -2623,7 +2626,7 @@ TEST_F(HfDownloaderPullHfModel, DownloadImageGenModelWithLoRA) { ASSERT_TRUE(std::filesystem::exists(graphPath)) << graphPath; // Verify LoRA adapter was downloaded - std::string loraDir = ovms::FileSystem::joinPath({basePath, "loras", "juliensimon", "sd-pokemon-lora"}); + std::string loraDir = ovms::FileSystem::joinPath({basePath, "loras", "MohamedAhmedAE", "stable-diffusion-v1-5_lora_finetuning"}); auto loraFiles = searchFilesRecursively(loraDir, {"pytorch_lora_weights.safetensors"}); ASSERT_FALSE(loraFiles.empty()) << "LoRA .safetensors not found in: " << loraDir;