feat: add two-phase progress bar for large model operations by aftersnow · Pull Request #474 · modelpack/modctl

aftersnow · 2026-03-27T04:57:00Z

Summary

Fix progress bar showing 0kb/s during SHA256 hashing (build) and blob existence checks (push) for large model files
Build now shows: "Hashing layer" (SHA256 phase) → "Building layer" (storage write phase)
Push now shows: "Checking blob" (HEAD request) → "Pushing blob" (upload phase)
Fix data race on bar.msg between Complete() and mpb render goroutine (string → atomic.Value)
Fix TOCTOU race in Add() lock pattern (RLock→RUnlock→Lock → single Lock)

Changes

File	Change
`internal/pb/pb.go`	Fix data race + lock pattern, add `Reset` method
`internal/pb/pb_test.go`	13 new tests (functional, concurrency, error/idempotency)
`pkg/backend/build/hooks/hooks.go`	Add `OnHash` callback for digest computation
`pkg/backend/build/builder.go`	Wire `onHash` into `computeDigestAndSize` with `io.Copy(hash, reader)`
`pkg/backend/build/builder_test.go`	3 new tests for `computeDigestAndSize`
`pkg/backend/processor/base.go`	Wire `OnHash` + `Reset` in processor hooks
`pkg/backend/push.go`	Two-phase Checking/Pushing display

Test plan

go test ./internal/pb/... -race -v — 13 tests pass, race detector clean
go test ./pkg/backend/build/... -v — all builder tests pass
go test ./pkg/backend/processor/... -v — all processor tests pass
go test ./pkg/... -count=1 — full package suite pass
go build ./... — compiles successfully
Manual E2E: build + push >8GB model on remote test machine

Follow-up

#680 replaces the checking-phase transfer bar with an indeterminate spinner. It addresses the Copilot finding that the checking phase still rendered 0 B/s. This PR was merged before that fix landed on the branch.

gemini-code-assist

Code Review

This pull request introduces an --include flag for the modelfile generate command, allowing users to explicitly include files or directories (such as hidden files) that are normally skipped. The path filtering logic has been migrated to use the doublestar library, enabling support for recursive glob patterns. Additionally, the PR enhances progress bar tracking by adding 'Hashing' and 'Checking' phases and improves thread safety for progress messages using atomic values. Feedback suggests simplifying the hashing implementation in builder.go by using io.Copy directly with the hash object instead of a TeeReader and io.Discard.

…ions - Change progressBar.msg from string to atomic.Value to fix data race between Complete() and mpb render goroutine - Change Add() from RLock-check-RUnlock-Abort-Lock-write to full write lock to eliminate TOCTOU race on same-name bars - Add Reset() method as semantic alias for Add() to support phase transitions (Hashing -> Building, Checking -> Pushing) - Add 13 unit tests: 6 functional, 3 concurrency (-race), 4 error/idempotency Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com> Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

Add OnHashFunc type and OnHash field to Hooks struct. Default OnHash passes reader through unchanged. Add WithOnHash option function. Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com> Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

…ss tracking - computeDigestAndSize now accepts onHash callback to wrap reader with progress tracking before SHA256 hashing via TeeReader - BuildLayer computes relPath once and passes it to both onHash (via closure) and OutputLayer, ensuring bar name consistency across phases - Add 3 tests: onHash called with correct size, wrapped reader preserves digest correctness, reader failure propagation Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com> Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

- Add WithOnHash hook calling tracker.Add("Hashing layer") for digest computation phase - Change WithOnStart from tracker.Add to tracker.Reset("Building layer") for storage write phase transition Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com> Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

- Remove prompt parameter from pushIfNotExist, phases now hardcoded - Phase 1: show "Checking blob" with nil reader during dst.Exists() - Phase 2: reset to "Pushing blob" with content reader for upload - Add pb.Abort on Exists() and PullBlob() error paths - Manifest/config pushes unchanged (small payloads, no phases needed) Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com> Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

Simplify computeDigestAndSize by copying directly to the sha256 writer instead of using io.TeeReader + io.Discard. hash.Hash already implements io.Writer, making the tee unnecessary. Addresses review feedback on PR #474. Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

- builder: invoke OnError hook when digest computation fails so the "Hashing layer" bar is aborted instead of lingering on screen - push: remove now-dead prompt parameter from pushIfNotExist and derive per-type prompts (blob/config/manifest), so config shows "Checking/Pushing/Skipped config" instead of "... blob" - pb: document Add/Reset replace semantics for completed bars (mpb ignores Abort on completed bars; PopCompletedMode pops them out) - builder: document that the hashing bar total is approximate for tar-encoded layers - tests: add BuildLayer OnError-on-hash-failure and Reset-after- completed-phase coverage Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

Copilot

🟡 Changes recommended

The checking phase still displays a zero byte rate because it uses the standard transfer decorators with no reader.

1 open finding

Avoid zero transfer rate during indeterminate checking phase · New

What changed in this PR

Adds two-phase progress reporting for model hashing/building and blob checking/uploading, while improving progress-bar concurrency safety.

Changes:

Adds hash-phase hooks and phase resets.
Makes progress messages atomic and bar replacement synchronized.
Adds progress and digest tests.

File	Description
`internal/pb/pb.go`	Adds atomic messages and bar reset support.
`internal/pb/pb_test.go`	Tests progress behavior and concurrency.
`pkg/backend/build/hooks/hooks.go`	Adds the `OnHash` hook.
`pkg/backend/build/builder.go`	Reports digest-computation progress.
`pkg/backend/build/builder_test.go`	Tests hashing hooks and failures.
`pkg/backend/processor/base.go`	Wires hashing and building phases.
`pkg/backend/push.go`	Adds checking and pushing phases.

🧠 Review effort: Balanced

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

bergwolf

lgtm!

Conflicts resolved: - internal/pb/pb.go: keep main's atomic.Value message (#474) and drop the msgMu lock; Placeholder stores the message through the atomic value. - internal/pb/pb_test.go: keep main's test file and add the Placeholder tests, including the message concurrency test, on top of it. - pkg/backend/processor/base.go: keep the per-attempt retry context and add main's OnHash hook. - pkg/backend/push.go: keep the per-attempt retry context and main's pushIfNotExist signature without the prompt argument. Retry prompts use main's phase names (Pushing blob / Pushing config). Signed-off-by: Zhao Chen <zhaochen.zju@gmail.com>

gemini-code-assist Bot reviewed Mar 27, 2026

View reviewed changes

Comment thread pkg/backend/build/builder.go Outdated

chlins reviewed Apr 15, 2026

View reviewed changes

Comment thread cmd/modelfile/generate.go

aftersnow added 6 commits April 23, 2026 11:45

aftersnow force-pushed the feature/progress-bar-phases branch from 1f183ab to 70d8013 Compare April 23, 2026 03:47

bergwolf previously approved these changes Apr 28, 2026

View reviewed changes

Merge main into progress bar phases

3b852ed

Signed-off-by: Zhao Chen <winters.zc@antgroup.com>

aftersnow dismissed bergwolf’s stale review via 3b852ed May 6, 2026 06:17

bergwolf requested a balanced review from Copilot October 10, 2026 14:30

Copilot started reviewing on behalf of bergwolf October 10, 2026 14:30 View session

Copilot AI reviewed Oct 10, 2026

View reviewed changes

Comment thread pkg/backend/push.go

bergwolf approved these changes Oct 10, 2026

View reviewed changes

bergwolf merged commit 0188f70 into main Oct 10, 2026
6 checks passed

bergwolf deleted the feature/progress-bar-phases branch October 10, 2026 14:44

aftersnow mentioned this pull request Oct 10, 2026

feat(pb): use an indeterminate spinner for the push checking phase #680

Open

aftersnow mentioned this pull request Oct 10, 2026

feat: add dynamic timeout retry with file-size-based parameters #468

Open

5 of 6 tasks

Navigation Menu

Repository navigation

feat: add two-phase progress bar for large model operations - #474

feat: add two-phase progress bar for large model operations#474
bergwolf merged 8 commits into
mainfrom
feature/progress-bar-phases

aftersnow commented Mar 27, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

bergwolf left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

aftersnow commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

Follow-up

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

bergwolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aftersnow commented Mar 27, 2026 •

edited

Loading