chore: refactor for backoff v5#574
Conversation
📝 WalkthroughWalkthroughThis PR upgrades github.com/cenkalti/backoff to v5, replaces Runner's stored BackOff with duration fields and a RetryOptions() API, updates handler and apiwriter retry code to v5 patterns, adds runner retry tests, and adjusts an async controller test. ChangesBackoff v4 to v5 Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/discovery/runner.go (1)
42-46: ⚡ Quick winClamp
WithBackoffintervals to prevent potential hot retry loops on misconfiguration.This repo only calls
WithBackoff(...)frompkg/discovery/runner_test.gowith positive values, so the specific 0/negative risk doesn’t seem reachable from current production code. Still,RetryOptions()wiresr.initialInterval/r.maxIntervaldirectly intobackoff.NewExponentialBackOff(), so defensive normalization inWithBackoffis low-effort hardening.🔧 Proposed fix
func WithBackoff[InputEvent any, OutputEvent any](initialInterval time.Duration, maxInterval time.Duration, maxElapsedTime time.Duration) RunnerOption[InputEvent, OutputEvent] { return func(r *Runner[InputEvent, OutputEvent]) { + if initialInterval <= 0 { + initialInterval = backoff.DefaultInitialInterval + } + if maxInterval <= 0 { + maxInterval = backoff.DefaultMaxInterval + } + if maxInterval < initialInterval { + maxInterval = initialInterval + } + if maxElapsedTime < 0 { + maxElapsedTime = backoff.DefaultMaxElapsedTime + } r.initialInterval = initialInterval r.maxInterval = maxInterval r.maxElapsedTime = maxElapsedTime } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/discovery/runner.go` around lines 42 - 46, The WithBackoff option should defensively clamp its interval inputs to safe, positive values and ensure initialInterval <= maxInterval before assigning to the Runner fields; update the WithBackoff function to normalize/validate initialInterval, maxInterval and maxElapsedTime (e.g., replace non-positive values with sensible defaults and set initialInterval = min(initialInterval, maxInterval) or vice-versa) so that RetryOptions() which feeds r.initialInterval/r.maxInterval into backoff.NewExponentialBackOff() cannot create a hot retry loop from misconfiguration; modify WithBackoff (the RunnerOption returned) to perform these checks and assignments on r.initialInterval, r.maxInterval, and r.maxElapsedTime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/discovery/runner.go`:
- Around line 42-46: The WithBackoff option should defensively clamp its
interval inputs to safe, positive values and ensure initialInterval <=
maxInterval before assigning to the Runner fields; update the WithBackoff
function to normalize/validate initialInterval, maxInterval and maxElapsedTime
(e.g., replace non-positive values with sensible defaults and set
initialInterval = min(initialInterval, maxInterval) or vice-versa) so that
RetryOptions() which feeds r.initialInterval/r.maxInterval into
backoff.NewExponentialBackOff() cannot create a hot retry loop from
misconfiguration; modify WithBackoff (the RunnerOption returned) to perform
these checks and assignments on r.initialInterval, r.maxInterval, and
r.maxElapsedTime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f097c325-40ea-49df-bec4-0774cf777f9c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modpkg/controller/target_controller_test.gopkg/discovery/apiwriter/apiwriter.gopkg/discovery/handler/handler.gopkg/discovery/runner.gopkg/discovery/runner_test.go
Coverage Report for CI Build 26951845025Coverage increased (+0.6%) to 74.493%Details
Uncovered Changes
Coverage Regressions5 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
@coderabbitai: issue addressed. |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
| initialInterval time.Duration | ||
| maxInterval time.Duration | ||
| maxElapsedTime time.Duration |
There was a problem hiding this comment.
The context for the new params is lost. Should we put them in a struct like backoffConfig to give them a context? That would also help with checking the params in line 179, so that we can just check whether backoffConfig is nil. If not, I'd rename them at least to backoffInitialInterval, etc.
4 participants
What
Closes #150
Testing
Unit tests added.
Checklist
Summary by CodeRabbit
Chores
Tests