dualfroz
18ce81580f
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.
84 lines
4.5 KiB
Markdown
84 lines
4.5 KiB
Markdown
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.
|