Repository navigation
fix: make the download progress bar complete and stable on Windows and Linux - #495
linuxsuren-fs wants to merge 4 commits into
Conversation
…d Linux - multi-thread downloads now feed one aggregated progress bar instead of juggling N bars with relative cursor movement, which printed garbage (e.g. literal escape sequences) on the classic Windows console - print the threads/size line with a proper newline so the progress bar no longer overwrites it - unknown-size downloads (no Content-Length) render a spinner with the transferred bytes instead of showing nothing at all - show the transferred/total byte counters, the speed and the elapsed time, and leave the prompt on the next line once completed - remove the 6-second post-download sleeps and the raw ANSI cursor fixes - collect the part download errors instead of printing them during the download, and cancel the remaining parts once one part failed
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 85 |
| Duplication | 14 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
- on a terminal the multi-thread download now renders one bar per part (part 1/4, part 2/4, ...), all the bars and the relative cursor movements go through a single locked ANSI-aware renderer, so the lines never garble each other on Windows and Linux - a piped or redirected output falls back to one aggregated bar, keeping the logs free of cursor control sequences - fix the bars vanishing at the end: closing an already finished bar erases its final frame (the renderer clears the line first and skips the re-render), only the unfinished bars get finished now - hd get: treat an existing empty output file as a broken download and overwrite it without asking for --force - multi-thread download: do not leave a broken (e.g. empty) output file behind when concatenating the parts failed
- DownloadWithContinue retries a transfer that broke in the middle (e.g. an unexpected EOF) and resumes from the bytes that are already on the local disk via range requests, instead of failing the whole download - the retry count comes from the so far unused --max-attempts flag (default 10), plumbed through MultiThreadDownloader as well - client errors (404, 403) are not retried, server errors and rate limiting are; a canceled download is never retried - if the server ignores the range request on a resumed download (200 instead of 206), fail instead of appending the full body to the partial file (which would corrupt it) - the resumed bytes count into the progress bar, so it reflects the whole file instead of only the remaining part
|
Thanks for the thorough PR — the design (locked multi-bar renderer via go-ansi, aggregated bar for piped output, spinner for unknown size, 200-vs-206 corruption guard on resume) looks right, and the new tests in Two findings block the CI, plus smaller notes: 🔴 Blocking — CI red1.
2. 🟡 Important3. 4. Empty-file auto-overwrite ( 🟢 Suggestions5. Everything else in the diff (per-part bars registered before goroutine start for stable line order, only-owner-closes bar semantics, cancellation between attempts, byte counters) checks out. With 1–2 addressed (and ideally 3) this is mergeable from my side; merging stays with the maintainer. |
- lazyFileWriter: create the directory with 0750 and the file with 0600 (gosec G301/G302) - get_test: return a fresh response with a fresh body per round trip in the multi-threads case, the shared single-use body made the parallel parts race (TestRunE failed on the Linux CI); chmod the output only after a successful download so a chmod error cannot mask the download error - do not retry local filesystem errors (e.g. ENOSPC, EACCES), a resumed request cannot fix them - document the empty-output-file auto-overwrite in the --force flag help - keep the progress disabled when an injected indicator was not initialized, instead of falling back to standalone bars per part - gofmt the downloadOption struct fields (pre-existing misalignment)
|
Thanks a lot for the careful review! All findings are addressed in 1. gosec G301/G302 — fixed: the directory is created with 2. 3. 4. Empty-file auto-overwrite documentation — documented in the 5. 6. Wasteful replay when the part file does not exist — the 4xx short-circuit you suggest is already in place: 7.
|
What problems are fixed
The download progress bar was incomplete and unfriendly on both Windows and Linux, and a broken transfer failed the whole download:
\r\033[NA) written raw toos.Stdout. On the classic Windows console (no VT processing) those sequences were printed literally as garbage, and on any terminal thestart to download with N threads, size: ...line had no trailing newline, so the first bar immediately overwrote it. Errors were also printed from the downloading goroutines, tearing the bars apart.progressbarwas created withmax = 0when the server sends noContent-Length;Add64then errors on every write (max must be greater than 0), so nothing was ever rendered for the whole download (chunked responses,--show-progressedge cases).OptionShowCountwas never set), and the color description tag was malformed ([cyan][reset]with an empty colored segment).\033[%dE) before continuing, and the completed bar never moved the cursor to the next line, so follow-up log lines appended to the bar.unexpected EOF), the download just failed, although the already transferred bytes were on the disk.hd getblocked on a broken empty output file. An interrupted multi-thread download could leave a 0-byte output file behind, and the next run refused to continue without--force. The--max-attemptsflag was not wired to anything.How it is fixed
Progress rendering
Interactive terminal: one progress bar per part (
MultiProgressIndicator). All the bars and the relative cursor movements go through a single locked renderer on the ANSI-aware output (go-ansi enables VT processing on Windows), so every part keeps its own stable line:Piped or redirected output: one aggregated bar fed by all the parts, which needs no cursor movements at all, so logs and CI output stay clean.
Unknown size (
Total <= 0) creates the bar in spinner mode (max = -1) which reports the transferred bytes and the speed for the whole download.The bars now show
transferred/total MB, speedand the elapsed time, print a newline on completion so the prompt starts on a fresh line, and rendering is throttled to 65ms.MultiThreadDownloaderprints the threads/size line with a proper newline (human readable size), collects part errors instead of printing them mid-download, cancels the remaining parts on the first failure, removes the 6-second sleeps and the raw ANSI fixups, and removes the broken output file when concatenating the parts failed.hd gettreats an existing empty output file as a broken download and overwrites it without--force(a non-empty file still requires--force).--show-progress=falseno longer creates any bar at all.Note: closing an already finished
progressbar/v3bar erases its final frame (the renderer clears the line first and then skips the re-render of a finished bar), so only the unfinished bars get finished when the group closes.Automatic retry and resume (断点续传)
DownloadWithContinueretries a transfer that broke in the middle (e.g.unexpected EOF) and resumes from the bytes that are already on the local disk: the part file is re-opened in append mode and the request continues withRange: bytes=<have>-<end>.--max-attemptsflag (default 10), now plumbed throughContinueDownloaderandMultiThreadDownloader(it applies per download part).ProgressBase), so a resumed bar reflects the whole file instead of only the remaining part.Verification
Unit tests (
pkg/net/progress_test.go,pkg/net/retry_resume_test.go): shared-bar aggregation (including a concurrent multi-part stress test), unknown-size handling, nil/no-bar passthrough, multi-bar line management (cursor up/down sequences, per-part byte counters, close semantics),humanBytesformatting, resume from a broken transfer (range headers, appended file content), no-retry on client errors, retry exhaustion, range-ignored protection against file corruption, cancelation while waiting, and the retryable-error classification.go test ./pkg/...passes (thepkg/installerOCI andpkg/osfailures are pre-existing on Windows, they fail on master too).Integration testing against a local HTTP server (Range support + a chunked no-Content-Length route + routes that break the first transfer in the middle):
script, 100 columns): the per-part bars render on dedicated lines and animate to 100% without garbling each other; with a part broken mid-transfer the part resumes automatically and still reaches 100% (visible via its lower average speed), final screen replayed through avt10xvirtual terminal.100%; a broken single-thread transfer and a broken multi-thread part both auto-resume and produce byte-identical files (SHA256 verified); chunked download shows the spinner with the transferred bytes;--show-progress=falseemits no escape sequences at all; a pre-existing 0-byte output file gets overwritten with a log line.