From 17f6069b0916eba9e5e271048e2ab745d8606248 Mon Sep 17 00:00:00 2001 From: Zhao Chen Date: Sat, 10 Oct 2026 22:56:43 +0800 Subject: [PATCH] feat(pb): use an indeterminate spinner for the push checking phase The checking phase created a transfer bar with a nil reader. That bar has byte counters and an AverageSpeed decorator, but nothing advances them, so a slow dst.Exists call rendered "0 B / " and "0 B/s" under "Checking ...", which is the zero transfer rate this PR removes. Add ProgressBar.Spinner: an indeterminate bar with a spinner filler, the prompt, the total size, and the elapsed time, and no byte or speed decorators. push uses it for the checking phase; Reset then replaces it with the transfer bar once the upload starts. Complete gives a spinner a total so mpb can mark it done. Tests cover creation, replacement by Reset, completion, the disabled path, and the rendered output (no counter, no rate), with a sanity test that a transfer bar does render the rate. Signed-off-by: Zhao Chen (cherry picked from commit 43820993b0f7c8314676d24d9a61cabc3c14a627) --- internal/pb/pb.go | 53 ++++++++++++++++++ internal/pb/pb_test.go | 124 +++++++++++++++++++++++++++++++++++++++++ pkg/backend/push.go | 7 ++- 3 files changed, 181 insertions(+), 3 deletions(-) diff --git a/internal/pb/pb.go b/internal/pb/pb.go index 8f0fb15e..ca0d2851 100644 --- a/internal/pb/pb.go +++ b/internal/pb/pb.go @@ -60,6 +60,9 @@ type progressBar struct { size int64 msg atomic.Value // stores string; accessed by mpb render goroutine startTime time.Time + // indeterminate marks a spinner bar created by Spinner. It has no byte + // counter and no completion trigger until Complete, Reset or Abort. + indeterminate bool } // NewProgressBar creates a new progress bar. @@ -141,6 +144,51 @@ func (p *ProgressBar) Add(prompt, name string, size int64, reader io.Reader) io. return reader } +// Spinner adds an indeterminate bar for a phase that transfers no bytes, such +// as a remote existence check. It renders a spinner, the prompt, the total +// size, and the elapsed time. It has no byte counter and no transfer rate, so +// a slow phase does not show a misleading "0 B / N B" and "0 B/s". Call Reset +// (or Add) with the same name to replace it with a transfer bar once bytes +// start flowing, or Complete / Abort to finish it. +func (p *ProgressBar) Spinner(prompt, name string, size int64) { + if disableProgress.Load() { + return + } + + p.mu.Lock() + defer p.mu.Unlock() + + // Same replacement semantics as Add. + if oldBar := p.bars[name]; oldBar != nil { + oldBar.Abort(true) + } + + newBar := &progressBar{ + size: size, + startTime: time.Now(), + indeterminate: true, + } + newBar.msg.Store(fmt.Sprintf("%s %s", prompt, name)) + + // A zero total disables mpb's completion trigger, so the bar keeps + // spinning until it is replaced, completed, or aborted. + newBar.Bar = p.mpb.New(0, + mpbv8.SpinnerStyle(), + mpbv8.PrependDecorators( + decor.Any(func(_ decor.Statistics) string { + return newBar.msg.Load().(string) + }, decor.WCSyncSpaceR), + ), + mpbv8.AppendDecorators( + decor.Name(humanize.Bytes(uint64(size)), decor.WCSyncWidthR), + decor.Name(" | ", decor.WCSyncWidthR), + decor.Elapsed(decor.ET_STYLE_GO, decor.WCSyncWidthR), + ), + ) + + p.bars[name] = newBar +} + // Get returns the progress bar. func (p *ProgressBar) Get(name string) *progressBar { p.mu.RLock() @@ -158,6 +206,11 @@ func (p *ProgressBar) Complete(name string, msg string) { if ok { bar.msg.Store(msg) + if bar.indeterminate { + // A spinner has no total; give it one so mpb marks it complete. + bar.SetTotal(bar.size, true) + return + } bar.Bar.SetCurrent(bar.size) } } diff --git a/internal/pb/pb_test.go b/internal/pb/pb_test.go index 782510d6..601ea004 100644 --- a/internal/pb/pb_test.go +++ b/internal/pb/pb_test.go @@ -23,11 +23,47 @@ import ( "strings" "sync" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// lockedBuffer is a bytes.Buffer that is safe for concurrent use. The mpb +// render goroutine writes frames while the test reads them. +type lockedBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *lockedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *lockedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// waitForOutput polls the buffer until it contains want or the timeout +// expires, and returns the final contents. +func waitForOutput(t *testing.T, out *lockedBuffer, want string, timeout time.Duration) string { + t.Helper() + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + if s := out.String(); strings.Contains(s, want) { + return s + } + time.Sleep(50 * time.Millisecond) + } + s := out.String() + require.Contains(t, s, want, "progress output never rendered %q", want) + return s +} + // --- Functional tests --- func TestAdd_WrapsReader(t *testing.T) { @@ -113,6 +149,94 @@ func TestReset_NoExistingBar(t *testing.T) { assert.Equal(t, "Building => new-file", bar.msg.Load().(string)) } +func TestSpinner_CreatesIndeterminateBar(t *testing.T) { + pb := NewProgressBar(io.Discard) + defer pb.Stop() + + pb.Spinner("Checking =>", "test-file", 100) + + bar := pb.Get("test-file") + require.NotNil(t, bar) + assert.True(t, bar.indeterminate) + assert.Equal(t, int64(100), bar.size) + assert.Equal(t, "Checking => test-file", bar.msg.Load().(string)) + assert.False(t, bar.Completed(), "spinner must not complete on its own") +} + +func TestSpinner_ResetSwitchesToTransferBar(t *testing.T) { + pb := NewProgressBar(io.Discard) + defer pb.Stop() + + pb.Spinner("Checking =>", "test-file", 100) + spinner := pb.Get("test-file") + require.NotNil(t, spinner) + + reader := pb.Reset("Pushing =>", "test-file", 100, strings.NewReader("data")) + require.NotNil(t, reader) + + bar := pb.Get("test-file") + require.NotNil(t, bar) + assert.NotSame(t, spinner, bar) + assert.False(t, bar.indeterminate) + assert.Equal(t, "Pushing => test-file", bar.msg.Load().(string)) + assert.True(t, spinner.Aborted(), "replaced spinner must be aborted") +} + +func TestSpinner_CompleteFinishesBar(t *testing.T) { + pb := NewProgressBar(io.Discard) + defer pb.Stop() + + pb.Spinner("Checking =>", "test-file", 100) + pb.Complete("test-file", "Skipped => test-file") + + bar := pb.Get("test-file") + require.NotNil(t, bar) + assert.Equal(t, "Skipped => test-file", bar.msg.Load().(string)) + assert.Eventually(t, bar.Completed, 2*time.Second, 20*time.Millisecond, + "Complete must finish an indeterminate bar") +} + +func TestSpinner_DisabledProgress(t *testing.T) { + SetDisableProgress(true) + defer SetDisableProgress(false) + + pb := NewProgressBar(io.Discard) + defer pb.Stop() + + pb.Spinner("Checking =>", "test-file", 100) + assert.Nil(t, pb.Get("test-file"), "no bar must be tracked when progress is disabled") +} + +func TestSpinner_RendersWithoutTransferRate(t *testing.T) { + out := &lockedBuffer{} + pb := NewProgressBar(out) + + pb.Spinner("Checking =>", "test-file", 100) + rendered := waitForOutput(t, out, "Checking => test-file", 3*time.Second) + pb.Stop() + + // The spinner shows the size and elapsed time, but no byte counter and + // no transfer rate. A transfer bar with no bytes flowing renders + // "0.00 b / 100.00 b | 0.00 b/s" (see TestAdd_RendersTransferRate). + assert.Contains(t, rendered, "100 B") + assert.NotContains(t, rendered, "b/s") + assert.NotContains(t, rendered, "0.00 b / 100.00 b") +} + +func TestAdd_RendersTransferRate(t *testing.T) { + out := &lockedBuffer{} + pb := NewProgressBar(out) + + pb.Add("Pushing =>", "test-file", 100, nil) + rendered := waitForOutput(t, out, "Pushing => test-file", 3*time.Second) + pb.Stop() + + // Sanity check for the assertion above: a transfer bar does render the + // counter and the rate, so the spinner test is not vacuous. + assert.Contains(t, rendered, "0.00 b / 100.00 b") + assert.Contains(t, rendered, "0.00 b/s") +} + // --- Concurrency tests (must pass go test -race) --- func TestAdd_ConcurrentSameName(t *testing.T) { diff --git a/pkg/backend/push.go b/pkg/backend/push.go index f5aa792c..ff3ef3af 100644 --- a/pkg/backend/push.go +++ b/pkg/backend/push.go @@ -152,10 +152,11 @@ func pushIfNotExist(ctx context.Context, pb *internalpb.ProgressBar, src storage // Phase 1: show "Checking" during the existence check. The manifest is // excluded since its payload is already in memory and pushed together - // with the tag right after. Bar is created with a nil reader so it - // indicates waiting state without transferring bytes. + // with the tag right after. The check transfers no bytes, so use an + // indeterminate spinner instead of a transfer bar that would sit at + // "0 B / N B" and "0 B/s" for the whole HEAD request. if desc.MediaType != ocispec.MediaTypeImageManifest { - pb.Add(internalpb.NormalizePrompt("Checking "+kind), desc.Digest.String(), desc.Size, nil) + pb.Spinner(internalpb.NormalizePrompt("Checking "+kind), desc.Digest.String(), desc.Size) } // check whether the content exists in the destination storage.