feat: Implement Retry Logic to Reduce Pkg Failures (#461)

* Added retry logic

* Updated scenario

* Addressed feedback

* network retries flag set correctly

Author: Antonio Salinas

internal/app/azldev/app.go

@@ -19,6 +19,7 @@ import (
 	"github.com/gim-home/azldev-preview/internal/global/opctx"
 	"github.com/gim-home/azldev-preview/internal/projectconfig"
 	"github.com/gim-home/azldev-preview/internal/utils/fileutils"
+	"github.com/gim-home/azldev-preview/internal/utils/retry"
 	"github.com/lmittmann/tint"
 	"github.com/mattn/go-isatty"
 	"github.com/muesli/termenv"
@@ -44,6 +45,7 @@ type App struct {
 	quiet                bool
 	acceptAllPrompts     bool
 	dryRun               bool
+	networkRetries       int
 	reportFormat         ReportFormat
 	disableDefaultConfig bool
 	configFiles          []string
@@ -124,6 +126,7 @@ func NewApp(fsFactory opctx.FileSystemFactory, osEnvFactory opctx.OSEnvFactory)
 			env.SetDefaultReportFormat(app.reportFormat)
 			env.SetAcceptAllPrompts(app.acceptAllPrompts)
 			env.SetColorMode(app.colorMode)
+			env.SetNetworkRetries(app.networkRetries)
 
 			return nil
 		},
@@ -156,6 +159,8 @@ func NewApp(fsFactory opctx.FileSystemFactory, osEnvFactory opctx.OSEnvFactory)
 	app.cmd.PersistentFlags().StringArrayVar(&app.configFiles, "config-file", nil,
 		"additional TOML config file(s) to merge (may be repeated)")
 	app.cmd.PersistentFlags().BoolVarP(&app.dryRun, "dry-run", "n", false, "dry run only (do not take action)")
+	app.cmd.PersistentFlags().IntVar(&app.networkRetries, "network-retries", retry.DefaultMaxAttempts,
+		"maximum number of attempts for network operations (minimum 1)")
 	app.cmd.PersistentFlags().VarP(&app.reportFormat, "output-format", "O",
 		"output format {csv, json, markdown, table}")
 	app.cmd.PersistentFlags().Var(&app.colorMode, "color",
@@ -289,7 +294,6 @@ func (a *App) Execute(args []string) int {
 	env := NewEnv(ctx, *envOptions)
 	env.SetVerbose(a.verbose)
 	env.SetQuiet(a.quiet)
-
 	env.SetColorMode(a.colorMode)
 	//
 	// If we managed to find a project + configuration, then we can let anyone who was

internal/app/azldev/env.go

@@ -65,6 +65,7 @@ type Env struct {
 	quiet               bool
 	promptsAllowed      bool
 	acceptAllPrompts    bool
+	networkRetries      int
 
 	// Injected dependencies.
 	cmdFactory    opctx.CmdFactory
@@ -171,6 +172,21 @@ func (env *Env) SetVerbose(verbose bool) {
 	env.verbose = verbose
 }
 
+// NetworkRetries returns the maximum number of attempts for network operations.
+func (env *Env) NetworkRetries() int {
+	return env.networkRetries
+}
+
+// SetNetworkRetries sets the maximum number of attempts for network operations.
+// Values less than 1 are clamped to 1.
+func (env *Env) SetNetworkRetries(retries int) {
+	if retries < 1 {
+		retries = 1
+	}
+
+	env.networkRetries = retries
+}
+
 // SetEventListener registers the event listener to be used in this environment.
 func (env *Env) SetEventListener(eventListener opctx.EventListener) {
 	env.eventListener = eventListener

internal/app/azldev/env_test.go

@@ -72,6 +72,28 @@ func TestNewEnv(t *testing.T) {
 	assert.Equal(t, env.Err(), ctx.Err())
 }
 
+func TestSetNetworkRetries(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    int
+		expected int
+	}{
+		{"positive value is preserved", 5, 5},
+		{"one is preserved", 1, 1},
+		{"zero is clamped to 1", 0, 1},
+		{"negative value is clamped to 1", -1, 1},
+		{"large negative is clamped to 1", -100, 1},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			testEnv := testutils.NewTestEnv(t)
+			testEnv.Env.SetNetworkRetries(tt.input)
+			assert.Equal(t, tt.expected, testEnv.Env.NetworkRetries())
+		})
+	}
+}
+
 func TestEnvConstructionTime(t *testing.T) {
 	testEnv := testutils.NewTestEnv(t)
 

internal/providers/sourceproviders/fedorasource/fedorasource.go

@@ -17,6 +17,7 @@ import (
 	"github.com/gim-home/azldev-preview/internal/global/opctx"
 	"github.com/gim-home/azldev-preview/internal/utils/downloader"
 	"github.com/gim-home/azldev-preview/internal/utils/fileutils"
+	"github.com/gim-home/azldev-preview/internal/utils/retry"
 )
 
 type FedoraSourceDownloader interface {
@@ -30,6 +31,7 @@ type FedoraSourceDownloaderImpl struct {
 	dryRunnable opctx.DryRunnable
 	fileSystem  opctx.FS
 	downloader  downloader.Downloader
+	retryConfig retry.Config
 }
 
 // Ensure [FedoraSourceDownloaderImpl] implements [FedoraSourceDownloader].
@@ -80,6 +82,7 @@ func NewFedoraRepoExtractorImpl(
 	dryRunnable opctx.DryRunnable,
 	fileSystem opctx.FS,
 	downloader downloader.Downloader,
+	retryCfg retry.Config,
 ) (*FedoraSourceDownloaderImpl, error) {
 	if fileSystem == nil {
 		return nil, errors.New("filesystem cannot be nil")
@@ -97,6 +100,7 @@ func NewFedoraRepoExtractorImpl(
 		dryRunnable: dryRunnable,
 		fileSystem:  fileSystem,
 		downloader:  downloader,
+		retryConfig: retryCfg,
 	}, nil
 }
 
@@ -181,12 +185,22 @@ func (g *FedoraSourceDownloaderImpl) downloadAndVerifySources(
 			"destPath", destFilePath,
 		)
 
-		if err := g.downloader.Download(ctx, sourceFile.uri, destFilePath); err != nil {
-			return fmt.Errorf("failed to download from %#q to %#q:\n%w", sourceFile.uri, destFilePath, err)
-		}
+		if err := retry.Do(ctx, g.retryConfig, func() error {
+			// Remove any partially written file from a prior failed attempt.
+			_ = g.fileSystem.Remove(destFilePath)
+
+			if downloadErr := g.downloader.Download(ctx, sourceFile.uri, destFilePath); downloadErr != nil {
+				return fmt.Errorf("failed to download from %#q to %#q:\n%w",
+					sourceFile.uri, destFilePath, downloadErr)
+			}
+
+			if hashErr := g.validateDownloadedFile(destFilePath, sourceFile); hashErr != nil {
+				return fmt.Errorf("hash validation failed for %#q:\n%w", sourceFile.fileName, hashErr)
+			}
 
-		if err := g.validateDownloadedFile(destFilePath, sourceFile); err != nil {
-			return fmt.Errorf("hash validation failed for %#q:\n%w", sourceFile.fileName, err)
+			return nil
+		}); err != nil {
+			return fmt.Errorf("failed to retrieve source file %#q:\n%w", sourceFile.fileName, err)
 		}
 	}
 

internal/providers/sourceproviders/fedorasource/fedorasource_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/gim-home/azldev-preview/internal/global/testctx"
 	"github.com/gim-home/azldev-preview/internal/utils/downloader/downloader_test"
 	"github.com/gim-home/azldev-preview/internal/utils/fileutils"
+	"github.com/gim-home/azldev-preview/internal/utils/retry"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -51,7 +52,7 @@ func TestNewFedoraRepoExtractorImpl(t *testing.T) {
 	ctx := testctx.NewCtx()
 	mockDownloader := downloader_test.NewMockDownloader(ctrl)
 
-	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader)
+	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader, retry.Disabled())
 
 	require.NoError(t, err)
 	require.NotNil(t, extractor)
@@ -63,17 +64,17 @@ func TestNewFedoraRepoExtractorImplValidation(t *testing.T) {
 	mockDownloader := downloader_test.NewMockDownloader(ctrl)
 
 	t.Run("nil dryRunnable should fail", func(t *testing.T) {
-		extractor, err := NewFedoraRepoExtractorImpl(nil, ctx.FS(), mockDownloader)
+		extractor, err := NewFedoraRepoExtractorImpl(nil, ctx.FS(), mockDownloader, retry.Disabled())
 		require.Error(t, err)
 		require.Nil(t, extractor)
 	})
 	t.Run("nil filesystem should fail", func(t *testing.T) {
-		extractor, err := NewFedoraRepoExtractorImpl(ctx, nil, mockDownloader)
+		extractor, err := NewFedoraRepoExtractorImpl(ctx, nil, mockDownloader, retry.Disabled())
 		require.Error(t, err)
 		require.Nil(t, extractor)
 	})
 	t.Run("nil downloader should fail", func(t *testing.T) {
-		extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), nil)
+		extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), nil, retry.Disabled())
 		require.Error(t, err)
 		require.Nil(t, extractor)
 	})
@@ -84,7 +85,7 @@ func TestExtractSourcesFromRepo(t *testing.T) {
 	ctx := testctx.NewCtx()
 	mockDownloader := downloader_test.NewMockDownloader(ctrl)
 
-	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader)
+	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader, retry.Disabled())
 	require.NoError(t, err)
 
 	require.NoError(t, ctx.FS().MkdirAll(testRepoDir, testDirPerms))
@@ -118,7 +119,7 @@ func TestExtractSourcesFromRepoValidation(t *testing.T) {
 	ctx := testctx.NewCtx()
 	mockDownloader := downloader_test.NewMockDownloader(ctrl)
 
-	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader)
+	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader, retry.Disabled())
 	require.NoError(t, err)
 
 	t.Run("empty repo dir", func(t *testing.T) {
@@ -147,7 +148,7 @@ func TestExtractSourcesFromRepoDownloadFailure(t *testing.T) {
 	ctx := testctx.NewCtx()
 	mockDownloader := downloader_test.NewMockDownloader(ctrl)
 
-	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader)
+	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader, retry.Disabled())
 	require.NoError(t, err)
 
 	require.NoError(t, ctx.FS().MkdirAll(testRepoDir, testDirPerms))
@@ -168,7 +169,7 @@ func TestExtractSourcesFromRepoHashMismatch(t *testing.T) {
 	ctx := testctx.NewCtx()
 	mockDownloader := downloader_test.NewMockDownloader(ctrl)
 
-	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader)
+	extractor, err := NewFedoraRepoExtractorImpl(ctx, ctx.FS(), mockDownloader, retry.Disabled())
 	require.NoError(t, err)
 
 	require.NoError(t, ctx.FS().MkdirAll(testRepoDir, testDirPerms))

internal/providers/sourceproviders/fedorasourceprovider.go

@@ -18,6 +18,7 @@ import (
 	"github.com/gim-home/azldev-preview/internal/providers/sourceproviders/fedorasource"
 	"github.com/gim-home/azldev-preview/internal/utils/fileutils"
 	"github.com/gim-home/azldev-preview/internal/utils/git"
+	"github.com/gim-home/azldev-preview/internal/utils/retry"
 )
 
 // DistroResolver is a function type that resolves a distro reference to its definition and version.
@@ -32,6 +33,7 @@ type FedoraSourcesProviderImpl struct {
 	gitProvider    git.GitProvider
 	downloader     fedorasource.FedoraSourceDownloader
 	distroResolver DistroResolver
+	retryConfig    retry.Config
 }
 
 var _ ComponentSourceProvider = (*FedoraSourcesProviderImpl)(nil)
@@ -42,6 +44,7 @@ func NewFedoraSourcesProviderImpl(
 	gitProvider git.GitProvider,
 	downloader fedorasource.FedoraSourceDownloader,
 	distroResolver DistroResolver,
+	retryCfg retry.Config,
 ) (*FedoraSourcesProviderImpl, error) {
 	if fs == nil {
 		return nil, errors.New("filesystem cannot be nil")
@@ -69,6 +72,7 @@ func NewFedoraSourcesProviderImpl(
 		gitProvider:    gitProvider,
 		downloader:     downloader,
 		distroResolver: distroResolver,
+		retryConfig:    retryCfg,
 	}, nil
 }
 
@@ -113,38 +117,48 @@ func (g *FedoraSourcesProviderImpl) GetComponent(
 
 	defer fileutils.RemoveAllAndUpdateErrorIfNil(g.fs, tempDir, &err)
 
-	// Clone the repository to temp directory
-	err = g.gitProvider.Clone(
-		ctx,
-		gitRepoURL,
-		tempDir,
-		git.WithGitBranch(distroGitBranch))
+	// Clone the repository to temp directory with retry for transient network failures.
+	err = retry.Do(ctx, g.retryConfig, func() error {
+		// Clean up temp directory contents from any prior failed clone attempt.
+		_ = g.fs.RemoveAll(tempDir)
+		_ = fileutils.MkdirAll(g.fs, tempDir)
+
+		return g.gitProvider.Clone(ctx, gitRepoURL, tempDir, git.WithGitBranch(distroGitBranch))
+	})
 	if err != nil {
 		return fmt.Errorf("failed to clone git repository %#q:\n%w", gitRepoURL, err)
 	}
 
+	// Process the cloned repo: checkout target commit, extract sources, copy to destination.
+	return g.processClonedRepo(ctx, effectiveDistroRef, tempDir, upstreamNameToUse, componentName,
+		lookasideBaseURI, destDirPath)
+}
+
+// processClonedRepo handles the post-clone steps: checking out the target commit,
+// extracting lookaside sources, renaming spec files, and copying to the destination.
+func (g *FedoraSourcesProviderImpl) processClonedRepo(
+	ctx context.Context,
+	effectiveDistroRef projectconfig.DistroReference,
+	tempDir, upstreamName, componentName, lookasideBaseURI, destDirPath string,
+) error {
 	// Checkout the appropriate commit based on component/distro config
-	err = g.checkoutTargetCommit(ctx, effectiveDistroRef, tempDir)
-	if err != nil {
+	if err := g.checkoutTargetCommit(ctx, effectiveDistroRef, tempDir); err != nil {
 		return fmt.Errorf("failed to checkout target commit:\n%w", err)
 	}
 
 	// Delete the .git directory so it's not copied to destination.
-	err = g.fs.RemoveAll(filepath.Join(tempDir, ".git"))
-	if err != nil {
+	if err := g.fs.RemoveAll(filepath.Join(tempDir, ".git")); err != nil {
 		return fmt.Errorf("failed to remove .git directory from cloned repository at %#q:\n%w",
 			tempDir, err)
 	}
 
 	// Extract sources from repo (downloads lookaside files into the temp dir)
-	err = g.downloader.ExtractSourcesFromRepo(ctx, tempDir, upstreamNameToUse, lookasideBaseURI)
-	if err != nil {
+	if err := g.downloader.ExtractSourcesFromRepo(ctx, tempDir, upstreamName, lookasideBaseURI); err != nil {
 		return fmt.Errorf("failed to extract sources from git repository:\n%w", err)
 	}
 
 	// If the upstream name differs from the component name, rename the spec in temp dir.
-	err = g.renameSpecIfNeeded(tempDir, upstreamNameToUse, componentName)
-	if err != nil {
+	if err := g.renameSpecIfNeeded(tempDir, upstreamName, componentName); err != nil {
 		return err
 	}
 
@@ -157,8 +171,7 @@ func (g *FedoraSourcesProviderImpl) GetComponent(
 		FileFilter: fileutils.SkipExistingFiles,
 	}
 
-	err = fileutils.CopyDirRecursive(g.dryRunnable, g.fs, tempDir, destDirPath, copyOptions)
-	if err != nil {
+	if err := fileutils.CopyDirRecursive(g.dryRunnable, g.fs, tempDir, destDirPath, copyOptions); err != nil {
 		return fmt.Errorf("failed to copy files to destination:\n%w", err)
 	}