Skip to content

Fix progress bar getting permanently stuck after an overshooting Add/Set call - #239

Open
dualfroz wants to merge 1 commit into
schollz:mainfrom
dualfroz:dualfroz/fix-add-exceeds-max-corruption
Open

Fix progress bar getting permanently stuck after an overshooting Add/Set call#239
dualfroz wants to merge 1 commit into
schollz:mainfrom
dualfroz:dualfroz/fix-add-exceeds-max-corruption

Conversation

@dualfroz

@dualfroz dualfroz commented Sep 5, 2026

Copy link
Copy Markdown

Problem

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.

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.
@dualfroz
dualfroz force-pushed the dualfroz/fix-add-exceeds-max-corruption branch from 05e18ff to 7408154 Compare September 5, 2026 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant