Refactor: Replace `WaitNext` usages with `Do` in backoff package to avoid panics

[Nsanjayboruds]

What happened:
The codebase currently contains direct usages of the WaitNext function in the pkg/backoff package. There is a TODO comment at pkg/backoff/backoff.go:65 indicating that using WaitNext directly can lead to panics in certain scenarios, and we need to refactor these usages.

What you expected to happen:
To avoid potential panics and improve stability, all usages of WaitNext outside of the pkg/backoff package should be replaced with the safer Do method. The Do method encapsulates the retry logic and executes operations safely without risking panics from improper loop handling.

How to reproduce it:
This is a proactive code refactoring task rather than a runtime bug reproduction.

1. Check pkg/backoff/backoff.go line 65 to see the TODO.
2. Search the codebase for invocations of WaitNext(ctx) on retry interfaces/objects to see where refactoring is needed.

Environment:

• piped version: N/A (Codebase refactoring)
• control-plane version: N/A
• Others: N/A

[krishpinto]

Hi @Nsanjayboruds , can I work on this issue?

[krishpinto]

Hmm, looks like #6996 alr covers. I'll look for something else.
Thanks.

[armistcxy]

To avoid potential panics and improve stability, all usages of WaitNext outside of the pkg/backoff package should be replaced with the safer Do method

I can't imagine how this could create panic, can you elaborate more on this.

WaitNext function:

func (r *retry) WaitNext(ctx context.Context) bool {
	defer func() {
		r.calls++
	}()

	if r.calls >= r.max {
		return false
	}

	d := r.backoff.Next()
	if d == 0 {
		select {
		case <-ctx.Done():
			return false
		default:
			return true
		}
	}

	t := time.NewTimer(d)
	select {
	case <-ctx.Done():
		return false
	case <-t.C:
		return true
	}
}

[Nsanjayboruds]

To avoid potential panics and improve stability, all usages of WaitNext outside of the pkg/backoff package should be replaced with the safer Do method

I can't imagine how this could create panic, can you elaborate more on this.

WaitNext function:

func (r *retry) WaitNext(ctx context.Context) bool {
defer func() {
r.calls++
}()

if r.calls >= r.max {
return false
}

d := r.backoff.Next()
if d == 0 {
select {
case <-ctx.Done():
return false
default:
return true
}
}

t := time.NewTimer(d)
select {
case <-ctx.Done():
return false
case <-t.C:
return true
}
}

You're right that WaitNext itself doesn't contain any panic-inducing logic.

The motivation comes from the TODO in pkg/backoff (introduced in PR #2290). The issue isn't that WaitNext is inherently unsafe, but that the imperative retry pattern it encourages is easier to misuse.

With WaitNext, callers typically write code like:

var resp *Response
for retry.WaitNext(ctx) {
    resp, err = client.Call(...)
    if err == nil {
        break
    }
}

In this style, state is shared outside the retry loop. If the loop exits before producing a valid result (for example, due to context cancellation or exhausting retries), it's easier for callers to accidentally use partially initialized or nil values unless every exit path is handled carefully. Historically, this pattern contributed to bugs, which is why the TODO recommends moving callers to Do.

Using retry.Do encapsulates the retry loop and keeps the operation and its result within a single closure. It encourages handling success and failure through the returned error instead of relying on variables whose initialization depends on loop execution. Besides making the code cleaner, it reduces the chance of mistakes such as accidentally using uninitialized state after the retry loop.

So the goal of this refactoring is primarily to improve API safety and consistency, rather than because WaitNext itself directly panics.

[armistcxy]

So why you framed it as a potential threat with panic behavior at first ?

[Nsanjayboruds]

So why you framed it as a potential threat with panic behavior at first ?

Looking back, I agree the wording was stronger than it should have been.

I initially framed it that way because of the existing TODO in pkg/backoff/backoff.go (replace by Do to avoid panic) and interpreted it as the motivation for the refactoring. After digging deeper, I realized the issue isn't that WaitNext itself panics, but that the imperative retry pattern it encourages makes it easier to accidentally use partially initialized or nil state after the retry loop.

The actual motivation is improving API safety and encouraging a safer usage pattern, not fixing a panic in WaitNext itself. I'll update the issue/PR title and description to reflect that more accurately. Thanks for pointing it out.