Conversation
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.
Contributor
There was a problem hiding this comment.
🟡 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 == 0whiledlnow > 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.
Comment on lines
+161
to
+164
| initResult = curl_global_init(CURL_GLOBAL_DEFAULT); | ||
| if (initResult == CURLE_OK) { | ||
| std::atexit([]() { curl_global_cleanup(); }); | ||
| } |
Comment on lines
+422
to
+425
| const size_t chunkSize = std::min<size_t>(4096, body.size() - offset); | ||
| sink.write(body.data() + offset, chunkSize); | ||
| return true; | ||
| }); |
Comment on lines
+441
to
+443
| 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); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛠 Summary
JIRA 194742
ISSUE 4550
Contribution: #4551
🧪 Checklist
``