Cvs 194742 progress bar fix by rasapala · Pull Request #4560 · openvinotoolkit/model_server

rasapala · 2026-09-14T13:35:31Z

🛠 Summary

JIRA 194742
ISSUE 4550
Contribution: #4551

🧪 Checklist

Unit tests added.
The documentation updated.
Change follows security best practices.
``

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.

Copilot

🟡 Changes recommended

The cURL cleanup conflict and test reliability issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes download progress handling for unknown or inaccurate content lengths, adds regression coverage, and updates LoRA test fixtures.

Changes:

Bounds progress-bar calculations and handles zero totals.
Adds chunked-transfer and progress tests.
Updates LoRA repositories and cURL initialization logic.

File summaries

File	Summary and review findings
`third_party/libgit2/lfs.patch`	Clamps the embedded LFS progress bar length.
`src/test/pull_hf_model_test.cpp`	Adds regression tests and updates fixtures. Moderate (3 votes): tests may not force a progress tick, and the timeout assertion can block while awaiting the async task.
`src/pull_module/curl_downloader.hpp`	Declares the progress-bar helper and testable arithmetic.
`src/pull_module/curl_downloader.cpp`	Handles unknown totals and safe bar sizing. Critical (2 votes): introduces an uncoordinated cURL lifetime owner. Nit (1 vote): unknown-size transfers do not emit a terminating newline.

Review details

Suppressed comments (1)

src/pull_module/curl_downloader.cpp:81

For an unknown-size transfer this branch returns without ever writing a terminating newline. Since the callback keeps dltotal == 0 while dlnow > 0, the final progress text is left on the same line and the subsequent model-complete message or shell prompt is concatenated with it. Track completion at the call site and emit a newline only once the transfer finishes.

        printf("\rProgress: %.2f %s downloaded, total size unknown", received, sizeUnits[receivedUnitId]);
        print_download_speed_info(count, elapsed_time);
        fflush(stdout);
        return;

Files reviewed: 4/4 changed files
Comments generated: 3
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

mzegla · 2026-09-15T08:38:41Z

+        return 0;
+    }
+    const double ratio = static_cast<double>(count) / static_cast<double>(max);
+    // Written as a positive test so a NaN ratio also lands here rather than falling through.


"Written as positive test"?

mzegla · 2026-09-15T08:39:46Z

+    }
+    const double ratio = static_cast<double>(count) / static_cast<double>(max);
+    // Written as a positive test so a NaN ratio also lands here rather than falling through.
+    if (!(ratio > 0.0)) {


Why not if (ratio <= 0.0)

mzegla · 2026-09-15T08:40:05Z

+    if (!(ratio > 0.0)) {
+        return 0;
+    }
+    if (ratio >= 1.0) {


Could be else if with above condition?

mzegla · 2026-09-15T08:42:58Z

+    // 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.


Looks like leftover comment from development process. It's hard for me to read as this comment seems to assume I have knowledge about some external context "...Content-Length reports dltotal == 0...".

mzegla · 2026-09-15T08:51:30Z


+// 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.


Suggested change

// no ratio to render and the result is 0. Declared here so the arithmetic can be unit tested.

// no ratio to render and the result is 0.

Unnecessary overhead. If it's not natural, we should change it. If it is, no need for a comment.

mzegla · 2026-09-15T08:54:08Z

+// 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.


I don't think we need it here.

mzegla · 2026-09-15T08:54:34Z

+// 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.


Repetition of the comment above function signature.

mzegla · 2026-09-15T08:57:18Z

 #include <filesystem>
 #include <iostream>
 #include <memory>
+#include <mutex>


Why do we need to include mutex in this PR?

std::once_flag and std::call_once

atobiszei · 2026-09-18T09:04:17Z

+// Keep one balanced libcurl global initialization for this downloader's process lifetime.
+// The previous per-call guard held a null unique_ptr, so its deleter never ran and every
+// download added another unmatched curl_global_init() call.
+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;
+}


This means that every unit test that did start OVMS before did curl init and curl cleanup. So ovms behaves now more differently in ovms gtest scenarios than in prod. Why could we not just move this initialization to pull module?

Why we can't follow pattern used in httpservermodule.cpp?
Init curl in module init, cleanup in shutdown?

Then we could dispose whole part of curl_global_init from functions used here/.

First question - yes.
We could follow the HttpServerModule init/shutdown pattern for the cases where HfPullModelModule::start()/shutdown() actually runs — but curl_downloader's functions aren't only invoked through that module's lifecycle. The curl_downloader.cpp-local std::call_once + atexit guard exists precisely to make the downloader self-sufficient regardless of how it's invoked. That said, it is true this changes gtest behavior (every test that exercises this file now does one process-wide curl_global_init/atexit-cleanup pair instead of zero) — but that's a correctness fix, not a new inconsistency: previously the guard was broken.

It is used only in pull_module. The other place we use it is:
https://github.com/openvinotoolkit/model_server/blob/main/src/llm/io_processing/image_utils.cpp#L68

but its entirely different curl call with their own guards.

Why did previous solution not work?
If there was issue with guards it could be reused solution from image_utils if you want to keep it self-contained. If we want to keep it called only once (which is not required by curl) then we could have it pull module.

Daksha1611 and others added 3 commits September 12, 2026 03:03

Add tets and fix patch

e31e15f

Fix segfault on error

14d3f3c

rasapala requested review from atobiszei and mzegla and a lite review from Copilot and removed request for Copilot September 14, 2026 13:35

Copilot started reviewing on behalf of rasapala September 14, 2026 13:36 View session

Merge branch 'main' into CVS-194742_progress_bar

1fa4c0e

Copilot AI lite review requested due to automatic review settings September 14, 2026 15:41

Copilot started reviewing on behalf of rasapala September 14, 2026 15:42 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Comment thread src/pull_module/curl_downloader.cpp

Comment thread src/test/pull_hf_model_test.cpp

Comment thread src/test/pull_hf_model_test.cpp Outdated

Merge branch 'main' into CVS-194742_progress_bar

d3279b0

mzegla reviewed Sep 15, 2026

View reviewed changes

Code review

018f715

mzegla reviewed Sep 15, 2026

View reviewed changes

rasapala added 4 commits September 15, 2026 11:10

Code review2

9e0bf98

Merge branch 'main' into CVS-194742_progress_bar

16e1199

Fix windows build

a608871

Merge branch 'main' into CVS-194742_progress_bar

5f43cbb

mzegla approved these changes Sep 16, 2026

View reviewed changes

Clean proxy

8b72d76

atobiszei reviewed Sep 18, 2026

View reviewed changes

rasapala added 2 commits September 18, 2026 14:58

Merge branch 'main' into CVS-194742_progress_bar

6d6fac1

Remove comments

69fdb7c

	// no ratio to render and the result is 0. Declared here so the arithmetic can be unit tested.
	// no ratio to render and the result is 0.

Conversation

rasapala commented Sep 14, 2026

🛠 Summary

🧪 Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants