Skip to content

Commit 23a54c1

Browse files
authored
Fix exponential backoff overflow race (#41)
Fixes a pre-existing concurrency bug in `exponentialBackoff.Next`, and folds in the modernization follow-ups that missed the #40 merge. ## Overflow race `Next` incremented the attempt counter with `Add(1)`, and on overflow decremented it again (`Add(^uint64(0))`) to un-count the attempt. That increment-then-decrement is not atomic as a unit, so under concurrent calls the counter transiently runs past the overflow point and `base << attempt` wraps to a bogus positive instead of saturating at `MaxInt64`. The counter now advances with a compare-and-swap that only commits when the shift does not overflow. Once it does, `Next` saturates without advancing, so concurrent callers converge on `MaxInt64` deterministically. This flake predates the Go 1.25 work: `TestExponentialBackoff/overflow` failed ~8/100 runs under `-race` on the merged `main`, ~14/100 on the modernization branch (the atomic-type swap was equivalent, so the rate is unchanged within noise). With the fix, the overflow tests pass 0 failures across 60+ `-race` runs. A new `TestExponentialBackoff_ConcurrentOverflow` fires 100 concurrent `Next` calls and asserts every result stays on the doubling sequence or `MaxInt64`. ## Modernization follow-ups These were staged during the self-review of #40 but did not land before it merged: - README: the randomization note still described `math/rand` seeded from the Unix timestamp; it is now `math/rand/v2` with an automatically seeded top-level generator. - Removed the blank lines left behind when the `tc := tc` copies were dropped. - Converted the index-free goroutine loops in the concurrent tests to range-over-int. - Updated the jitter guard comments to reference `rand.Int64N` instead of the removed `Int63n`. --------- Signed-off-by: Seth Vargo <seth@sethvargo.com>
1 parent 2713be8 commit 23a54c1

6 files changed

Lines changed: 61 additions & 18 deletions

README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,8 @@ Benchmark/sethvargo-7 203,914,245 5.73 ns/op
178178

179179
## Notes and Caveats
180180

181-
- Randomization uses `math/rand` seeded with the Unix timestamp instead of
182-
`crypto/rand`.
181+
- Randomization uses `math/rand/v2` (non-cryptographic, automatically seeded),
182+
not `crypto/rand`.
183183
- Ordering of addition of multiple modifiers will make a difference.
184184
For example; ensure you add `CappedDuration` before `WithMaxDuration`, otherwise it may early out too early.
185185
Another example is you could add `Jitter` before or after capping depending on your desired outcome.

backoff_constant_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,14 +63,13 @@ func TestConstantBackoff(t *testing.T) {
6363
}
6464

6565
for _, tc := range cases {
66-
6766
t.Run(tc.name, func(t *testing.T) {
6867
t.Parallel()
6968

7069
b := retry.NewConstant(tc.base)
7170

7271
resultsCh := make(chan time.Duration, tc.tries)
73-
for i := 0; i < tc.tries; i++ {
72+
for range tc.tries {
7473
go func() {
7574
r, _ := b.Next()
7675
resultsCh <- r

backoff_exponential.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,15 @@ func NewExponential(base time.Duration) Backoff {
3737

3838
// Next implements Backoff. It is safe for concurrent use.
3939
func (b *exponentialBackoff) Next() (time.Duration, bool) {
40-
next := b.base << (b.attempt.Add(1) - 1)
41-
if next <= 0 {
42-
b.attempt.Add(^uint64(0))
43-
next = math.MaxInt64
44-
}
40+
for {
41+
attempt := b.attempt.Load()
42+
next := b.base << attempt
43+
if next <= 0 {
44+
return math.MaxInt64, false
45+
}
4546

46-
return next, false
47+
if b.attempt.CompareAndSwap(attempt, attempt+1) {
48+
return next, false
49+
}
50+
}
4751
}

backoff_exponential_test.go

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,14 +69,13 @@ func TestExponentialBackoff(t *testing.T) {
6969
}
7070

7171
for _, tc := range cases {
72-
7372
t.Run(tc.name, func(t *testing.T) {
7473
t.Parallel()
7574

7675
b := retry.NewExponential(tc.base)
7776

7877
resultsCh := make(chan time.Duration, tc.tries)
79-
for i := 0; i < tc.tries; i++ {
78+
for range tc.tries {
8079
go func() {
8180
r, _ := b.Next()
8281
resultsCh <- r
@@ -115,3 +114,47 @@ func ExampleNewExponential() {
115114
// 8s
116115
// 16s
117116
}
117+
118+
func TestExponentialBackoff_ConcurrentOverflow(t *testing.T) {
119+
t.Parallel()
120+
121+
// Many concurrent Next calls on an overflow-prone base must never observe a
122+
// value outside the doubling sequence or MaxInt64. A racy attempt counter
123+
// lets the shift run past the overflow point and wrap to a bogus positive.
124+
const tries = 100
125+
base := 100_000 * time.Hour
126+
127+
b := retry.NewExponential(base)
128+
129+
resultsCh := make(chan time.Duration, tries)
130+
for range tries {
131+
go func() {
132+
r, _ := b.Next()
133+
resultsCh <- r
134+
}()
135+
}
136+
137+
results := make([]time.Duration, tries)
138+
for i := range tries {
139+
select {
140+
case val := <-resultsCh:
141+
results[i] = val
142+
case <-time.After(5 * time.Second):
143+
t.Fatal("timeout")
144+
}
145+
}
146+
slices.Sort(results)
147+
148+
want := make([]time.Duration, 0, tries)
149+
for next := base; next > 0; next <<= 1 {
150+
want = append(want, next)
151+
}
152+
for len(want) < tries {
153+
want = append(want, math.MaxInt64)
154+
}
155+
slices.Sort(want)
156+
157+
if !reflect.DeepEqual(results, want) {
158+
t.Errorf("expected \n\n%v\n\n to be \n\n%v\n\n", results, want)
159+
}
160+
}

backoff_fibonacci_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,14 +81,13 @@ func TestFibonacciBackoff(t *testing.T) {
8181
}
8282

8383
for _, tc := range cases {
84-
8584
t.Run(tc.name, func(t *testing.T) {
8685
t.Parallel()
8786

8887
b := retry.NewFibonacci(tc.base)
8988

9089
resultsCh := make(chan time.Duration, tc.tries)
91-
for i := 0; i < tc.tries; i++ {
90+
for range tc.tries {
9291
go func() {
9392
r, _ := b.Next()
9493
resultsCh <- r

backoff_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,11 +84,10 @@ func TestWithJitter_NonPositive(t *testing.T) {
8484
}
8585

8686
for _, tc := range cases {
87-
8887
t.Run(tc.name, func(t *testing.T) {
8988
t.Parallel()
9089

91-
// A non-positive jitter must not panic (Int63n panics on a
90+
// A non-positive jitter must not panic (rand.Int64N panics on a
9291
// non-positive argument) and must return the underlying value
9392
// unchanged.
9493
b := retry.WithJitter(tc.j, retry.BackoffFunc(func() (time.Duration, bool) {
@@ -165,11 +164,10 @@ func TestWithJitterPercent_Zero(t *testing.T) {
165164
}
166165

167166
for _, tc := range cases {
168-
169167
t.Run(tc.name, func(t *testing.T) {
170168
t.Parallel()
171169

172-
// A zero jitter must not panic (Int63n panics on a non-positive
170+
// A zero jitter must not panic (rand.Int64N panics on a non-positive
173171
// argument) and must return the underlying value unchanged.
174172
b := retry.WithJitterPercent(tc.j, retry.BackoffFunc(func() (time.Duration, bool) {
175173
return tc.nextVal, tc.nextStop

0 commit comments

Comments
 (0)