Automatic Retry with Exponential Backoff #91
Conversation
This commit introduces a resilient, configurable retry mechanism for network requests. Key changes include: - A new `pkg/retry` package with a custom `http.Transport` that implements exponential backoff and jitter. - Integration of the retry transport into the `website`, `pwa`, and `github` packages to handle transient network failures gracefully. - New persistent CLI flags (`--retries`, `--retry-backoff`, `--retry-max`, `--retry-jitter`, `--no-retry`) to allow user configuration of the retry behavior. - The flag-handling logic has been moved to a `PersistentPreRun` function to ensure user-provided values are parsed correctly. - A basic retry mechanism has been added to the `vcs` package for git clone operations. - Added unit tests for the retry transport. This work is in progress, with the next steps being to implement support for the `Retry-After` header and unify the VCS retry logic with the global configuration. Co-authored-by: Snider <631881+Snider@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 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 |
Summary of ChangesHello @Snider, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's robustness by introducing a comprehensive retry mechanism for network operations. It centralizes retry logic within a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for automatic retries with exponential backoff, enhancing the application's resilience to transient network issues. The implementation is generally well-structured, with a new retry package that is integrated across various components. However, I've identified a critical bug in the retry logic that could lead to panics when retries are disabled. Additionally, there are several opportunities for improvement, including handling errors from CLI flags, addressing duplicated retry logic in one of the packages, and improving test coverage for the new functionality. My detailed comments below provide specific suggestions to address these points.
| for i := 0; i < t.Retries; i++ { | ||
| resp, err = t.transport().RoundTrip(req) | ||
| if err == nil && resp.StatusCode < 500 { | ||
| return resp, err | ||
| } | ||
|
|
||
| if i < t.Retries-1 { | ||
| time.Sleep(time.Duration(backoff)) | ||
| backoff = backoff.Exponential().Jitter(t.Jitter) | ||
| if backoff > Backoff(t.MaxBackoff) { | ||
| backoff = Backoff(t.MaxBackoff) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a critical issue in the retry logic. When t.Retries is 0 (for example, when the --no-retry flag is used), the for loop does not execute at all. This causes the function to return (nil, nil), which will lead to a panic in the calling code when it attempts to access the nil response. The logic should ensure that at least one request attempt is made, even if retries are disabled.
attempts := t.Retries
if attempts <= 0 {
attempts = 1
}
for i := 0; i < attempts; i++ {
resp, err = t.transport().RoundTrip(req)
if err == nil && resp.StatusCode < 500 {
return resp, err
}
if i < attempts-1 {
time.Sleep(time.Duration(backoff))
backoff = backoff.Exponential().Jitter(t.Jitter)
if backoff > Backoff(t.MaxBackoff) {
backoff = Backoff(t.MaxBackoff)
}
}
}| rootCmd.PersistentPreRun = func(cmd *cobra.Command, args []string) { | ||
| // Configure retry settings | ||
| retries, _ := cmd.Flags().GetInt("retries") | ||
| retryBackoff, _ := cmd.Flags().GetDuration("retry-backoff") | ||
| retryMax, _ := cmd.Flags().GetDuration("retry-max") | ||
| retryJitter, _ := cmd.Flags().GetFloat64("retry-jitter") | ||
| noRetry, _ := cmd.Flags().GetBool("no-retry") | ||
|
|
||
| if noRetry { | ||
| retry.DefaultTransport.Retries = 0 | ||
| } else { | ||
| retry.DefaultTransport.Retries = retries | ||
| retry.DefaultTransport.InitialBackoff = retryBackoff | ||
| retry.DefaultTransport.MaxBackoff = retryMax | ||
| retry.DefaultTransport.Jitter = retryJitter | ||
| } | ||
| } |
There was a problem hiding this comment.
Ignoring errors from flag parsing can lead to silent failures and unexpected behavior if a flag is ever renamed or removed. For better robustness, these errors should be handled. You can switch from PersistentPreRun to PersistentPreRunE, which allows you to return an error that cobra will then handle and display to the user.
rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
// Configure retry settings
retries, err := cmd.Flags().GetInt("retries")
if err != nil {
return err
}
retryBackoff, err := cmd.Flags().GetDuration("retry-backoff")
if err != nil {
return err
}
retryMax, err := cmd.Flags().GetDuration("retry-max")
if err != nil {
return err
}
retryJitter, err := cmd.Flags().GetFloat64("retry-jitter")
if err != nil {
return err
}
noRetry, err := cmd.Flags().GetBool("no-retry")
if err != nil {
return err
}
if noRetry {
retry.DefaultTransport.Retries = 0
} else {
retry.DefaultTransport.Retries = retries
retry.DefaultTransport.InitialBackoff = retryBackoff
retry.DefaultTransport.MaxBackoff = retryMax
retry.DefaultTransport.Jitter = retryJitter
}
return nil
}|
|
||
| "github.com/Snider/Borg/cmd" | ||
| "github.com/Snider/Borg/pkg/logger" | ||
| "github.com/Snider/Borg/pkg/retry" |
There was a problem hiding this comment.
This import of the retry package is not used in main.go and is redundant, as the cmd package already imports and configures it. This line can be removed.
| package retry | ||
|
|
||
| import ( | ||
| "math/rand" |
There was a problem hiding this comment.
The math/rand package is used for jitter but it is not explicitly seeded. This can result in the same sequence of random numbers on each program run, making the jitter deterministic. While Go 1.20 and later automatically seed the global random number generator, it's a good practice to explicitly seed it for compatibility with older Go versions. You can add an init() function to the package for this purpose:
func init() {
rand.Seed(time.Now().UnixNano())
}| func TestTransport_RoundTrip(t *testing.T) { | ||
| var requestCount int | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| requestCount++ | ||
| if requestCount <= 2 { | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| transport := NewTransport() | ||
| transport.Retries = 3 | ||
| transport.InitialBackoff = 1 * time.Millisecond | ||
| transport.MaxBackoff = 10 * time.Millisecond | ||
|
|
||
| client := NewClient(transport) | ||
| req, _ := http.NewRequest("GET", server.URL, nil) | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| t.Errorf("expected status code %d, got %d", http.StatusOK, resp.StatusCode) | ||
| } | ||
|
|
||
| if requestCount != 3 { | ||
| t.Errorf("expected 3 requests, got %d", requestCount) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test for RoundTrip covers the success scenario well. To ensure the robustness of the retry mechanism, it would be beneficial to add test cases for edge scenarios. For instance, you could add tests for:
- When
Retriesis 0 or 1, to verify the correct number of attempts are made. - When all retry attempts fail, to ensure the last error and response are returned correctly.
| var lastErr error | ||
| retries := 3 | ||
| backoff := 1 * time.Second | ||
| maxBackoff := 30 * time.Second | ||
|
|
||
| for i := 0; i < retries; i++ { | ||
| _, err = git.PlainClone(tempPath, false, cloneOptions) | ||
| if err == nil { | ||
| lastErr = nil | ||
| break | ||
| } | ||
|
|
||
| lastErr = err | ||
|
|
||
| // Handle non-retryable error | ||
| if err.Error() == "remote repository is empty" { | ||
| return datanode.New(), nil | ||
| } | ||
| return nil, err | ||
|
|
||
| // Don't wait on the last attempt | ||
| if i < retries-1 { | ||
| time.Sleep(backoff) | ||
| backoff *= 2 | ||
| if backoff > maxBackoff { | ||
| backoff = maxBackoff | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if lastErr != nil { | ||
| return nil, lastErr | ||
| } |
There was a problem hiding this comment.
This file introduces a manual implementation of a retry loop with exponential backoff. Since the pull request's main purpose is to add a centralized retry package, this duplicated logic should be avoided. To improve consistency and maintainability, consider refactoring this to use a more generic retry utility. For example, you could extend the retry package with a generic Do function that can wrap non-HTTP operations like git.PlainClone.
This submission adds a new feature for automatic retries with exponential backoff for network requests. It includes a new
retrypackage, CLI flags for configuration, and integration with existing packages. The work is in progress based on recent code review feedback.Fixes #52
PR created automatically by Jules for task 16602402838586068838 started by @Snider