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.
This commit is contained in:
1 parent
ab40861d8a
commit
18ce81580f
3 files changed
+116
-3
No files matched your search
@@ -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.
|
||||
Reference in new issue
Block a user