feat: adopt an exponential backoff as a retry mechanism and integrate…

… review findings

cmd/dex/config.go

@@ -5,10 +5,12 @@ import (
 	"encoding/json"
 	"fmt"
 	"log/slog"
+	"math"
 	"net/http"
 	"net/netip"
 	"os"
 	"strings"
+	"time"
 
 	"golang.org/x/crypto/bcrypt"
 
@@ -78,6 +80,13 @@ func (c Config) Validate() error {
 		{c.GRPC.TLSMaxVersion != "" && c.GRPC.TLSMinVersion != "" && c.GRPC.TLSMinVersion > c.GRPC.TLSMaxVersion, "TLSMinVersion greater than TLSMaxVersion"},
 	}
 
+	if err := c.Storage.Retry.Validate(); err != nil {
+		checks = append(checks, struct {
+			bad    bool
+			errMsg string
+		}{true, err.Error()})
+	}
+
 	var checkErrors []string
 
 	for _, check := range checks {
@@ -254,10 +263,53 @@ type GRPC struct {
 
 // Storage holds app's storage configuration.
 type Storage struct {
-	Type          string        `json:"type"`
-	Config        StorageConfig `json:"config"`
-	RetryAttempts int           `json:"retryAttempts"`
-	RetryDelay    string        `json:"retryDelay"`
+	Type   string        `json:"type"`
+	Config StorageConfig `json:"config"`
+	Retry  Retry         `json:"retry"`
+}
+
+// Retry holds retry mechanism configuration.
+type Retry struct {
+	MaxAttempts   int           `json:"maxAttempts"`
+	InitialDelay  time.Duration `json:"initialDelay"`
+	MaxDelay      time.Duration `json:"maxDelay"`
+	BackoffFactor float64       `json:"backoffFactor"`
+}
+
+func (r *Retry) Validate() error {
+	// If retry is configured but empty, return an error
+	if r.MaxAttempts == 0 && r.InitialDelay == 0 && r.MaxDelay == 0 && r.BackoffFactor == 0 {
+		return fmt.Errorf("empty configuration is supplied for storage retry")
+	}
+
+	if r.MaxAttempts < 1 {
+		return fmt.Errorf("storage retry max attempts must be at least 1")
+	}
+
+	// TODO type of durations?
+	initialDelay, err := time.ParseDuration(r.InitialDelay.String())
+	if err != nil || initialDelay <= 0 {
+		return fmt.Errorf("storage retry initial delay must be a positive duration")
+	}
+
+	maxDelay, err := time.ParseDuration(r.MaxDelay.String())
+	if err != nil || maxDelay <= 0 {
+		return fmt.Errorf("storage retry max delay must be a positive duration")
+	}
+
+	if maxDelay < initialDelay {
+		return fmt.Errorf("storage retry max delay must be greater than or equal to initial delay")
+	}
+
+	if r.BackoffFactor <= 1 {
+		return fmt.Errorf("storage retry backoff factor must be greater than 1")
+	}
+	// exponential backoff algorithm-specific check
+	if float64(maxDelay) < float64(initialDelay)*math.Pow(r.BackoffFactor, float64(r.MaxAttempts-1)) {
+		return fmt.Errorf("storage retry max delay is too small for the given initial delay, backoff factor, and max attempts")
+	}
+
+	return nil
 }
 
 // StorageConfig is a configuration that can create a storage.
@@ -299,10 +351,9 @@ var storages = map[string]func() StorageConfig{
 // dynamically determine the type of the storage config.
 func (s *Storage) UnmarshalJSON(b []byte) error {
 	var store struct {
-		Type          string          `json:"type"`
-		Config        json.RawMessage `json:"config"`
-		RetryAttempts int             `json:"retryAttempts"`
-		RetryDelay    string          `json:"retryDelay"`
+		Type   string          `json:"type"`
+		Config json.RawMessage `json:"config"`
+		Retry  Retry           `json:"retry"`
 	}
 	if err := json.Unmarshal(b, &store); err != nil {
 		return fmt.Errorf("parse storage: %v", err)
@@ -323,11 +374,37 @@ func (s *Storage) UnmarshalJSON(b []byte) error {
 			return fmt.Errorf("parse storage config: %v", err)
 		}
 	}
+
+	// TODO when retry is empty?
+	maxAttempts := store.Retry.MaxAttempts
+	if maxAttempts == 0 {
+		maxAttempts = 5
+	}
+	// TODO maybe only check for empty string?
+	initialDelay, err := time.ParseDuration(store.Retry.InitialDelay.String())
+	if err != nil {
+		initialDelay = time.Second
+	}
+
+	maxDelay, err := time.ParseDuration(store.Retry.MaxDelay.String())
+	if err != nil {
+		maxDelay = 5 * time.Minute
+	}
+
+	backoffFactor := store.Retry.BackoffFactor
+	if backoffFactor == 0 {
+		backoffFactor = 2
+	}
+
 	*s = Storage{
-		Type:          store.Type,
-		Config:        storageConfig,
-		RetryAttempts: store.RetryAttempts,
-		RetryDelay:    store.RetryDelay,
+		Type:   store.Type,
+		Config: storageConfig,
+		Retry: Retry{
+			MaxAttempts:   maxAttempts,
+			InitialDelay:  initialDelay,
+			MaxDelay:      maxDelay,
+			BackoffFactor: backoffFactor,
+		},
 	}
 	return nil
 }

cmd/dex/config_test.go

@@ -4,10 +4,10 @@ import (
 	"log/slog"
 	"os"
 	"testing"
+	"time"
 
 	"github.com/ghodss/yaml"
 	"github.com/kylelemons/godebug/pretty"
-	"github.com/stretchr/testify/require"
 
 	"github.com/dexidp/dex/connector/mock"
 	"github.com/dexidp/dex/connector/oidc"
@@ -26,6 +26,12 @@ func TestValidConfiguration(t *testing.T) {
 			Config: &sql.SQLite3{
 				File: "examples/dex.db",
 			},
+			Retry: Retry{
+				MaxAttempts:   10,
+				InitialDelay:  1 * time.Second,
+				MaxDelay:      512 * time.Second,
+				BackoffFactor: 2,
+			},
 		},
 		Web: Web{
 			HTTP: "127.0.0.1:5556",
@@ -55,7 +61,8 @@ func TestInvalidConfiguration(t *testing.T) {
 	wanted := `invalid Config:
 	-	no issuer specified in config file
 	-	no storage supplied in config file
-	-	must supply a HTTP/HTTPS  address to listen on`
+	-	must supply a HTTP/HTTPS  address to listen on
+	-	empty configuration is supplied for storage retry`
 	if got != wanted {
 		t.Fatalf("Expected error message to be %q, got %q", wanted, got)
 	}
@@ -153,6 +160,12 @@ additionalFeatures: [
 					ConnectionTimeout: 3,
 				},
 			},
+			Retry: Retry{
+				MaxAttempts:   5,
+				InitialDelay:  0 * time.Second,
+				MaxDelay:      0 * time.Second,
+				BackoffFactor: 2,
+			},
 		},
 		Web: Web{
 			HTTPS:         "127.0.0.1:5556",
@@ -371,6 +384,12 @@ logger:
 					ConnectionTimeout: 3,
 				},
 			},
+			Retry: Retry{
+				MaxAttempts:   5,
+				InitialDelay:  0 * time.Second,
+				MaxDelay:      0 * time.Second,
+				BackoffFactor: 2,
+			},
 		},
 		Web: Web{
 			HTTP: "127.0.0.1:5556",
@@ -449,22 +468,23 @@ logger:
 	}
 }
 
-func TestUnmarshalConfigWithRetry(t *testing.T) {
-	rawConfig := []byte(`
-storage:
-  type: postgres
-  config:
-    host: 10.0.0.1
-    port: 65432
-  retryAttempts: 10
-  retryDelay: "1s"
-`)
-
-	var c Config
-	err := yaml.Unmarshal(rawConfig, &c)
-	require.NoError(t, err)
-
-	require.Equal(t, "postgres", c.Storage.Type)
-	require.Equal(t, 10, c.Storage.RetryAttempts)
-	require.Equal(t, "1s", c.Storage.RetryDelay)
-}
+// func TestUnmarshalConfigWithRetry(t *testing.T) {
+// 	rawConfig := []byte(`
+// storage:
+//   type: postgres
+//   config:
+//     host: 10.0.0.1
+//     port: 65432
+//   retry:
+//     attempts: 10
+//     delay: 1s
+// `)
+
+// 	var c Config
+// 	err := yaml.Unmarshal(rawConfig, &c)
+// 	require.NoError(t, err)
+
+// 	require.Equal(t, "postgres", c.Storage.Type)
+// 	require.Equal(t, 10, c.Storage.Retry.Attempts)
+// 	require.Equal(t, "1s", c.Storage.Retry.Delay)
+// }

cmd/dex/serve.go

@@ -695,31 +695,34 @@ func initializeStorageWithRetry(storageConfig Storage, logger *slog.Logger) (sto
 	var s storage.Storage
 	var err error
 
-	retryAttempts := storageConfig.RetryAttempts
-	if retryAttempts == 0 {
-		retryAttempts = 5 // Default to 5 attempts
-	}
+	maxAttempts := storageConfig.Retry.MaxAttempts
+	initialDelay := storageConfig.Retry.InitialDelay
+	maxDelay := storageConfig.Retry.MaxDelay
+	backoffFactor := storageConfig.Retry.BackoffFactor
 
-	retryDelay, err := time.ParseDuration(storageConfig.RetryDelay)
-	if err != nil {
-		retryDelay = 5 * time.Second // Default to 5 seconds
-	}
+	delay := initialDelay
 
-	for attempt := 1; attempt <= retryAttempts; attempt++ {
+	for attempt := 1; attempt <= maxAttempts; attempt++ {
 		s, err = storageConfig.Config.Open(logger)
 		if err == nil {
 			return s, nil
 		}
 
 		logger.Error("Failed to initialize storage",
-			"attempt", fmt.Sprintf("%d/%d", attempt, retryAttempts),
+			"attempt", fmt.Sprintf("%d/%d", attempt, maxAttempts),
 			"error", err)
 
-		if attempt < retryAttempts {
+		if attempt < maxAttempts {
 			logger.Info("Retrying storage initialization",
-				"nextAttemptIn", retryDelay.String())
-			time.Sleep(retryDelay)
+				"nextAttemptIn", delay.String())
+			time.Sleep(delay)
+
+			// Calculate next delay using exponential backoff
+			delay = time.Duration(float64(delay) * backoffFactor)
+			if delay > maxDelay {
+				delay = maxDelay
+			}
 		}
 	}
-	return nil, fmt.Errorf("failed to initialize storage after %d attempts: %v", retryAttempts, err)
+	return nil, fmt.Errorf("failed to initialize storage after %d attempts: %v", maxAttempts, err)
 }

cmd/dex/serve_test.go

@@ -1,15 +1,14 @@
 package main
 
 import (
-	"context"
-	"errors"
+	"fmt"
 	"log/slog"
 	"testing"
-
-	"github.com/stretchr/testify/require"
+	"time"
 
 	"github.com/dexidp/dex/storage"
 	"github.com/dexidp/dex/storage/memory"
+	"github.com/stretchr/testify/require"
 )
 
 func TestNewLogger(t *testing.T) {
@@ -32,11 +31,7 @@ func TestNewLogger(t *testing.T) {
 		require.Equal(t, (*slog.Logger)(nil), logger)
 	})
 }
-
 func TestStorageInitializationRetry(t *testing.T) {
-	_, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
 	// Create a mock storage that fails a certain number of times before succeeding
 	mockStorage := &mockRetryStorage{
 		failuresLeft: 3,
@@ -45,10 +40,14 @@ func TestStorageInitializationRetry(t *testing.T) {
 	config := Config{
 		Issuer: "http://127.0.0.1:5556/dex",
 		Storage: Storage{
-			Type:          "mock",
-			Config:        mockStorage,
-			RetryAttempts: 5,
-			RetryDelay:    "1s",
+			Type:   "mock",
+			Config: mockStorage,
+			Retry: Retry{
+				MaxAttempts:   5,
+				InitialDelay:  time.Second,
+				MaxDelay:      10 * time.Second,
+				BackoffFactor: 2,
+			},
 		},
 		Web: Web{
 			HTTP: "127.0.0.1:5556",
@@ -59,7 +58,8 @@ func TestStorageInitializationRetry(t *testing.T) {
 		},
 	}
 
-	logger, _ := newLogger(config.Logger.Level, config.Logger.Format)
+	logger, err := newLogger(config.Logger.Level, config.Logger.Format)
+	require.NoError(t, err)
 
 	s, err := initializeStorageWithRetry(config.Storage, logger)
 	require.NoError(t, err)
@@ -75,7 +75,7 @@ type mockRetryStorage struct {
 func (m *mockRetryStorage) Open(logger *slog.Logger) (storage.Storage, error) {
 	if m.failuresLeft > 0 {
 		m.failuresLeft--
-		return nil, errors.New("mock storage failure")
+		return nil, fmt.Errorf("mock storage failure")
 	}
 	return memory.New(logger), nil
 }