Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"context"
"log/slog"

"github.com/Snider/Borg/pkg/retry"
"github.com/spf13/cobra"
)

Expand All @@ -16,6 +17,30 @@
}

rootCmd.PersistentFlags().BoolP("verbose", "v", false, "Enable verbose logging")
rootCmd.PersistentFlags().Int("retries", 3, "Max retry attempts")
rootCmd.PersistentFlags().Duration("retry-backoff", 1s, "Initial backoff duration")

Check failure on line 21 in cmd/root.go

View workflow job for this annotation

GitHub Actions / build

syntax error: unexpected name s in argument list; possibly missing comma or )

Check failure on line 21 in cmd/root.go

View workflow job for this annotation

GitHub Actions / build

syntax error: unexpected name s in argument list; possibly missing comma or )
rootCmd.PersistentFlags().Duration("retry-max", 30s, "Maximum backoff duration")

Check failure on line 22 in cmd/root.go

View workflow job for this annotation

GitHub Actions / build

syntax error: unexpected name s in argument list; possibly missing comma or )

Check failure on line 22 in cmd/root.go

View workflow job for this annotation

GitHub Actions / build

syntax error: unexpected name s in argument list; possibly missing comma or )
rootCmd.PersistentFlags().Float64("retry-jitter", 0.1, "Randomization factor")
rootCmd.PersistentFlags().Bool("no-retry", false, "Disable retries")

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
}
}
Comment on lines +26 to +42

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
	}


return rootCmd
}

Expand Down
1 change: 1 addition & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/Snider/Borg/cmd"
"github.com/Snider/Borg/pkg/logger"
"github.com/Snider/Borg/pkg/retry"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

)

var osExit = os.Exit
Expand Down
10 changes: 9 additions & 1 deletion pkg/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"os"
"strings"

"github.com/Snider/Borg/pkg/retry"
"golang.org/x/oauth2"
)

Expand All @@ -30,12 +31,19 @@ type githubClient struct{}
// NewAuthenticatedClient creates a new authenticated http client.
var NewAuthenticatedClient = func(ctx context.Context) *http.Client {
token := os.Getenv("GITHUB_TOKEN")

// Create a base client with retry transport.
baseClient := &http.Client{Transport: retry.NewTransport()}

if token == "" {
return http.DefaultClient
return baseClient
}

ts := oauth2.StaticTokenSource(
&oauth2.Token{AccessToken: token},
)

ctx = context.WithValue(ctx, oauth2.HTTPClient, baseClient)
return oauth2.NewClient(ctx, ts)
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/github/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"testing"

"github.com/Snider/Borg/pkg/mocks"
"github.com/Snider/Borg/pkg/retry"
)

func TestGetPublicRepos_Good(t *testing.T) {
Expand Down Expand Up @@ -164,8 +165,8 @@ func TestNewAuthenticatedClient_Bad(t *testing.T) {
// Unset the variable to ensure it's not present
t.Setenv("GITHUB_TOKEN", "")
client := NewAuthenticatedClient(context.Background())
if client != http.DefaultClient {
t.Error("expected http.DefaultClient when no token is set, but got something else")
if _, ok := client.Transport.(*retry.Transport); !ok {
t.Errorf("expected transport to be *retry.Transport, but got %T", client.Transport)
}
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/github/release.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net/http"
"strings"

"github.com/Snider/Borg/pkg/retry"
"github.com/google/go-github/v39/github"
)

Expand All @@ -22,12 +23,12 @@ var (
return http.NewRequest(method, url, body)
}
// DefaultClient is the default http client
DefaultClient = &http.Client{}
DefaultClient = retry.NewClient(retry.NewTransport())
)

// GetLatestRelease gets the latest release for a repository.
func GetLatestRelease(owner, repo string) (*github.RepositoryRelease, error) {
client := NewClient(nil)
client := NewClient(NewAuthenticatedClient(context.Background()))
release, _, err := client.Repositories.GetLatestRelease(context.Background(), owner, repo)
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/pwa/pwa.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"sync"

"github.com/Snider/Borg/pkg/datanode"
"github.com/Snider/Borg/pkg/retry"
"github.com/schollz/progressbar/v3"
"golang.org/x/net/html"
)
Expand All @@ -32,7 +33,7 @@ type PWAClient interface {

// NewPWAClient creates a new PWAClient.
func NewPWAClient() PWAClient {
return &pwaClient{client: http.DefaultClient}
return &pwaClient{client: retry.NewClient(retry.NewTransport())}
}

type pwaClient struct {
Expand Down
99 changes: 99 additions & 0 deletions pkg/retry/client.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
package retry

import (
"math/rand"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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())
}

"net/http"
"time"
)

// Backoff is a time.Duration that represents the backoff strategy.
type Backoff time.Duration

// Exponential returns a new Backoff duration that is the current duration
// multiplied by 2.
func (b Backoff) Exponential() Backoff {
return Backoff(time.Duration(b) * 2)
}

// Jitter returns a new Backoff duration with a random jitter added.
func (b Backoff) Jitter(factor float64) Backoff {
if factor <= 0 {
return b
}
jitter := time.Duration(rand.Float64() * factor * float64(b))
return Backoff(time.Duration(b) + jitter)
}

// Transport is an http.RoundTripper that automatically retries requests.
type Transport struct {
// Transport is the underlying http.RoundTripper to use for requests.
// If nil, http.DefaultTransport is used.
Transport http.RoundTripper

// Retries is the maximum number of retries to attempt.
Retries int

// InitialBackoff is the initial backoff duration.
InitialBackoff time.Duration

// MaxBackoff is the maximum backoff duration.
MaxBackoff time.Duration

// Jitter is the jitter factor to apply to backoff durations.
Jitter float64
}

// NewTransport creates a new Transport with default values.
func NewTransport() *Transport {
return &Transport{
Transport: http.DefaultTransport,
Retries: 3,
InitialBackoff: 1 * time.Second,
MaxBackoff: 30 * time.Second,
Jitter: 0.1,
}
}

// RoundTrip implements the http.RoundTripper interface.
func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
var resp *http.Response
var err error
var backoff = Backoff(t.InitialBackoff)

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)
}
}
}
Comment on lines +63 to +76

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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)
			}
		}
	}

return resp, err
}

func (t *Transport) transport() http.RoundTripper {
if t.Transport != nil {
return t.Transport
}
return http.DefaultTransport
}

// NewClient returns a new http.Client that uses the Transport.
func NewClient(transport *Transport) *http.Client {
return &http.Client{
Transport: transport,
}
}

var (
// DefaultTransport is the default transport with retry logic.
DefaultTransport = NewTransport()
// DefaultClient is the default client that uses the default transport.
DefaultClient = NewClient(DefaultTransport)
)
42 changes: 42 additions & 0 deletions pkg/retry/client_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package retry

import (
"net/http"
"net/http/httptest"
"testing"
"time"
)

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)
}
}
Comment on lines +10 to +42

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 Retries is 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.

32 changes: 29 additions & 3 deletions pkg/vcs/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"io"
"os"
"path/filepath"
"time"

"github.com/Snider/Borg/pkg/datanode"

Expand Down Expand Up @@ -37,12 +38,37 @@ func (g *gitCloner) CloneGitRepository(repoURL string, progress io.Writer) (*dat
cloneOptions.Progress = progress
}

_, err = git.PlainClone(tempPath, false, cloneOptions)
if err != nil {
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
}
Comment on lines +41 to 72

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.


dn := datanode.New()
Expand Down
3 changes: 2 additions & 1 deletion pkg/website/website.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strings"

"github.com/Snider/Borg/pkg/datanode"
"github.com/Snider/Borg/pkg/retry"
"github.com/schollz/progressbar/v3"

"golang.org/x/net/html"
Expand All @@ -28,7 +29,7 @@ type Downloader struct {

// NewDownloader creates a new Downloader.
func NewDownloader(maxDepth int) *Downloader {
return NewDownloaderWithClient(maxDepth, http.DefaultClient)
return NewDownloaderWithClient(maxDepth, retry.NewClient(retry.NewTransport()))
}

// NewDownloaderWithClient creates a new Downloader with a custom http.Client.
Expand Down
Loading