From 18ce81580f814a24c00fc78cac172e8412f70d1c Mon Sep 17 00:00:00 2001 From: DualFroz Date: Fri, 4 Sep 2026 08:23:03 +0200 Subject: [PATCH] fix: do not corrupt state when Add/Set overshoots max Add64 checked currentNum > max only after already mutating currentNum and currentBytes, so a single call that overshoots max returned an error but left the bar stuck: currentNum frozen above max, percent stuck above 100%, and every later Add/Set call failing with the same error forever, since the top-of-function currentNum < max guard could never pass again. Check the bound before mutating state instead, so an overshooting call is rejected as a whole and leaves currentNum/currentBytes/currentPercent exactly as they were. The now-unreachable post-hoc check is removed. Add TestAddExceedsMaxDoesNotCorruptState covering the corrupted-state case and that the bar keeps accepting valid progress afterwards. --- PR.md | 83 +++++++++++++++++++++++++++++++++++++++++++++ progressbar.go | 7 ++-- progressbar_test.go | 29 ++++++++++++++++ 3 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 PR.md diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..1b358b3 --- /dev/null +++ b/PR.md @@ -0,0 +1,83 @@ +Title: Fix progress bar getting permanently stuck after an overshooting Add/Set call + +Problem +------- +`Add64` (progressbar.go:724, reached by `Add`, `Add64`, `Set` and `Set64`) is meant to reject a +call that would push the counter past `max` and leave the bar unchanged: + + if p.state.currentNum < p.config.max { + ... + p.state.currentNum += num + } + ... + if p.state.currentNum > p.config.max { + return errors.New("current number exceeds max") + } + +The bounds check runs after `currentNum` (and `currentBytes`) have already been mutated, so a +single call that overshoots `max` is not rejected atomically: the error is returned, but the +invalid, over-max state is kept. Once that happens the bar is stuck: `currentNum` never advances +again, because the guard at the top of the function (`currentNum < max`) is now permanently false, +and `currentPercent`/the saucer width stay frozen above 100%. Every subsequent `Add` or `Set` call, +even ones that would otherwise be perfectly valid, keeps returning "current number exceeds max" +forever, with no way to recover short of `Reset()`. + +Reproduced on a clean checkout with: + + bar := New(100) + bar.Add(95) // currentNum=95, no error + bar.Add(10) // currentNum=105, err="current number exceeds max" + bar.Add(1) // currentNum still 105, same error again -- stuck forever + +This is reachable through the public API with nothing exotic: any caller whose increments do not +sum to exactly `max` (a very common case, e.g. an estimated total that is off by a few bytes, or +a `Set()` racing another goroutine's `Add()`) will hit this on the final update, at 95-105% of the +way through, and never see the bar reach 100% or accept further progress again. + +Fix +--- +Move the bounds check before the mutation, so an overshooting call is rejected as a whole and +leaves `currentNum`/`currentBytes`/`currentPercent` exactly as they were before the call: + + if !p.config.ignoreLength && p.state.currentNum+num > p.config.max { + return errors.New("current number exceeds max") + } + +The `ignoreLength` mode is left out of the new guard on purpose: it advances with +`(currentNum + num) % max`, which by construction wraps and never exceeds `max`, so the check +would never trigger there anyway; excluding it keeps that code path untouched. The old post-hoc +check at the end of the function is now unreachable in the non-ignoreLength path (the mutation it +used to guard can no longer produce an out-of-range value) and is removed rather than left as dead +code. + +Behavior change worth flagging: previously, once `currentNum` reached exactly `max`, further +`Add(n)` calls with `n > 0` were silently absorbed with no error (currentNum simply stopped +advancing, though currentBytes kept drifting upward unnoticed). With this fix such calls now +consistently return the same "current number exceeds max" error as any other overshoot, which +matches what the error message already claims and is the only existing test's expectation +(`TestBar`, which only asserts that an error is returned, not any particular resulting state). + +Added `TestAddExceedsMaxDoesNotCorruptState` in progressbar_test.go, next to the existing `TestBar` +overshoot test, asserting that a rejected `Add` leaves `currentNum`/`currentPercent` unchanged and +that the bar keeps accepting valid progress afterwards. + +Verification +------------ +- `go build ./...`: exit 0. +- `go vet .` (this package): exit 0. `go vet ./...` (whole repo) exits 1, but solely because of a + pre-existing, unrelated issue in `examples/download-unknown/main.go:14` ("using resp before + checking for errors"), confirmed present before this change via `git stash`; not touched here. +- `gofmt -l .`: no output (clean). +- `go test ./... -count=1 -v`: exit 0, 33 top-level PASS, 0 FAIL, 0 SKIP. +- Counterfactual: reverting only the `Add64` fix (keeping the new test) fails it with + `expected currentNum to stay at 95 after a rejected Add, got 105`, matching the reproduction + above; re-applying the fix restores a clean pass. + +Flags for account owner +------------------------ +- The "silently absorbed at exactly max" -> "consistently rejected" behavior change described + above is the one substantive semantic change beyond the corruption fix; flagging it explicitly + in case the maintainer wants it called out in a changelog entry. +- Two unrelated open PRs (#235, #237) touch `progressbar.go`/`progressbar_test.go` in different + functions (`clearProgressBar`/`render` visibility); checked their diffs, no line overlap with + this change. diff --git a/progressbar.go b/progressbar.go index 4060005..79a8e6a 100644 --- a/progressbar.go +++ b/progressbar.go @@ -741,6 +741,10 @@ func (p *ProgressBar) Add64(num int64) error { return errors.New("max must be greater than 0") } + if !p.config.ignoreLength && p.state.currentNum+num > p.config.max { + return errors.New("current number exceeds max") + } + if p.state.currentNum < p.config.max { if p.config.ignoreLength { p.state.currentNum = (p.state.currentNum + num) % p.config.max @@ -774,9 +778,6 @@ func (p *ProgressBar) Add64(num int64) error { updateBar := p.state.currentPercent != p.state.lastPercent && p.state.currentPercent > 0 p.state.lastPercent = p.state.currentPercent - if p.state.currentNum > p.config.max { - return errors.New("current number exceeds max") - } // always update if show bytes/second or its/second if updateBar || p.config.showIterationsPerSecond || p.config.showIterationsCount { diff --git a/progressbar_test.go b/progressbar_test.go index acc7bbe..26ea494 100644 --- a/progressbar_test.go +++ b/progressbar_test.go @@ -420,6 +420,35 @@ func TestBar(t *testing.T) { } } +func TestAddExceedsMaxDoesNotCorruptState(t *testing.T) { + bar := NewOptions(100, OptionSetWriter(io.Discard)) + + if err := bar.Add(95); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if err := bar.Add(10); err == nil { + t.Error("should have an error for adding past max") + } + + // A rejected Add must leave state exactly as it was before the call, + // not partially applied, so the bar is not left stuck above 100%. + if bar.state.currentNum != 95 { + t.Errorf("expected currentNum to stay at 95 after a rejected Add, got %d", bar.state.currentNum) + } + if bar.state.currentPercent != 95 { + t.Errorf("expected currentPercent to stay at 95 after a rejected Add, got %d", bar.state.currentPercent) + } + + // The bar must still accept further, non-overshooting progress afterwards. + if err := bar.Add(5); err != nil { + t.Errorf("bar should still accept progress after a rejected Add, got error: %v", err) + } + if bar.state.currentNum != 100 { + t.Errorf("expected currentNum to reach 100, got %d", bar.state.currentNum) + } +} + func TestState(t *testing.T) { bar := NewOptions(100, OptionSetWidth(10)) bar.Reset()