fix(publicip): retry fetching information on failure

internal/publicip/data.go

@@ -13,6 +13,10 @@ func (l *Loop) GetData() (data models.PublicIP) {
 // ClearData is used when the VPN connection goes down
 // and the public IP is not known anymore.
 func (l *Loop) ClearData() (err error) {
+	// Stop retrying to fetch the public IP information until the
+	// next tunnel up trigger, since the VPN tunnel is going down.
+	l.retryWanted.Store(false)
+
 	l.ipDataMutex.Lock()
 	defer l.ipDataMutex.Unlock()
 	l.ipData = models.PublicIP{}

internal/publicip/loop.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/netip"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/qdm12/gluetun/internal/configuration/settings"
@@ -20,12 +21,19 @@ type Loop struct {
 	ipData        models.PublicIP
 	ipDataMutex   sync.RWMutex
 	fetcher       *api.ResilientFetcher
+	// retryWanted is true when the last fetch failed and a retry
+	// is scheduled. It is set to false when the VPN tunnel goes down
+	// (see ClearData) so retries stop until the next tunnel up trigger.
+	retryWanted atomic.Bool
 	// Fixed injected objects
 	httpClient *http.Client
 	logger     Logger
 	// Fixed parameters
 	puid int
 	pgid int
+	// Retry backoff parameters, set as fields for test purposes.
+	retryInitialBackoff time.Duration
+	retryMaxBackoff     time.Duration
 	// Internal channels and locks
 	// runCtx is used to detect when the loop has exited
 	// when performing an update
@@ -40,6 +48,11 @@ type Loop struct {
 	timeNow func() time.Time
 }
 
+const (
+	defaultRetryInitialBackoff = 10 * time.Second
+	defaultRetryMaxBackoff     = 5 * time.Minute
+)
+
 func NewLoop(settings settings.PublicIP, puid, pgid int,
 	httpClient *http.Client, logger Logger,
 ) (loop *Loop, err error) {
@@ -49,13 +62,15 @@ func NewLoop(settings settings.PublicIP, puid, pgid int,
 	}
 
 	return &Loop{
-		settings:   settings,
-		httpClient: httpClient,
-		fetcher:    api.NewResilient(fetchers, logger),
-		logger:     logger,
-		puid:       puid,
-		pgid:       pgid,
-		timeNow:    time.Now,
+		settings:            settings,
+		httpClient:          httpClient,
+		fetcher:             api.NewResilient(fetchers, logger),
+		logger:              logger,
+		puid:                puid,
+		pgid:                pgid,
+		retryInitialBackoff: defaultRetryInitialBackoff,
+		retryMaxBackoff:     defaultRetryMaxBackoff,
+		timeNow:             time.Now,
 	}, nil
 }
 
@@ -98,51 +113,93 @@ func (l *Loop) run(runCtx context.Context, runDone chan<- struct{},
 ) {
 	defer close(runDone)
 
+	retryBackoff := l.retryInitialBackoff
+	var retryTimer *time.Timer
+	var retryTimerC <-chan time.Time // nil when no retry is scheduled
+
+	scheduleRetry := func() {
+		l.retryWanted.Store(true)
+		if retryTimer == nil {
+			retryTimer = time.NewTimer(retryBackoff)
+		} else {
+			retryTimer.Reset(retryBackoff)
+		}
+		retryTimerC = retryTimer.C
+		l.logger.Info("retrying to fetch public IP information in " +
+			retryBackoff.String())
+		const backoffFactor = 2
+		retryBackoff *= backoffFactor
+		if retryBackoff > l.retryMaxBackoff {
+			retryBackoff = l.retryMaxBackoff
+		}
+	}
+	unscheduleRetry := func() {
+		if retryTimerC == nil {
+			return
+		}
+		if !retryTimer.Stop() {
+			<-retryTimer.C
+		}
+		retryTimerC = nil
+	}
+
 	for {
-		var singleRunCtx context.Context
-		var singleRunResult chan<- error
 		select {
 		case <-runCtx.Done():
 			return
-		case singleRunCtx = <-runTrigger:
+		case singleRunCtx := <-runTrigger:
 			// Note singleRunCtx is canceled if runCtx is canceled.
-			singleRunResult = runResult
+			// A new trigger supersedes any previously scheduled retry.
+			unscheduleRetry()
+			retryBackoff = l.retryInitialBackoff
+			if !*l.settings.Enabled {
+				runResult <- nil
+				continue
+			}
+			err := l.fetchAndStore(singleRunCtx)
+			if err != nil {
+				scheduleRetry()
+			}
+			runResult <- err
+		case <-retryTimerC:
+			retryTimerC = nil
+			if !l.retryWanted.Load() || !*l.settings.Enabled {
+				continue
+			}
+			err := l.fetchAndStore(runCtx)
+			if err != nil && l.retryWanted.Load() {
+				l.logger.Warn("fetching public IP information failed: " +
+					err.Error())
+				scheduleRetry()
+			}
 		case partialUpdate := <-updateTrigger:
-			var err error
-			err = l.update(partialUpdate)
-			updatedResult <- err
-			continue
-		}
-
-		if !*l.settings.Enabled {
-			singleRunResult <- nil
-			continue
-		}
-
-		result, err := l.fetcher.FetchInfo(singleRunCtx, netip.Addr{})
-		if err != nil {
-			err = fmt.Errorf("fetching information: %w", err)
-			singleRunResult <- err
-			continue
+			updatedResult <- l.update(partialUpdate)
 		}
+	}
+}
 
-		message := "Public IP address is " + result.IP.String()
-		message += " (" + result.Country + ", " + result.Region + ", " + result.City +
-			" - source: " + l.fetcher.String() + ")"
-		l.logger.Info(message)
+func (l *Loop) fetchAndStore(ctx context.Context) (err error) {
+	result, err := l.fetcher.FetchInfo(ctx, netip.Addr{})
+	if err != nil {
+		return fmt.Errorf("fetching information: %w", err)
+	}
 
-		l.ipDataMutex.Lock()
-		l.ipData = result
-		l.ipDataMutex.Unlock()
+	message := "Public IP address is " + result.IP.String()
+	message += " (" + result.Country + ", " + result.Region + ", " + result.City +
+		" - source: " + l.fetcher.String() + ")"
+	l.logger.Info(message)
 
-		filepath := *l.settings.IPFilepath
-		err = persistPublicIP(filepath, result.IP.String(), l.puid, l.pgid)
-		if err != nil {
-			err = fmt.Errorf("persisting public ip address: %w", err)
-		}
+	l.ipDataMutex.Lock()
+	l.ipData = result
+	l.ipDataMutex.Unlock()
 
-		singleRunResult <- err
+	filepath := *l.settings.IPFilepath
+	err = persistPublicIP(filepath, result.IP.String(), l.puid, l.pgid)
+	if err != nil {
+		return fmt.Errorf("persisting public ip address: %w", err)
 	}
+
+	return nil
 }
 
 func (l *Loop) RunOnce(ctx context.Context) (err error) {

internal/publicip/loop_test.go

@@ -0,0 +1,139 @@
+package publicip
+
+import (
+	"context"
+	"errors"
+	"net/netip"
+	"os"
+	"path/filepath"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/qdm12/gluetun/internal/configuration/settings"
+	"github.com/qdm12/gluetun/internal/models"
+	"github.com/qdm12/gluetun/internal/publicip/api"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+var errTest = errors.New("test error")
+
+// failingThenSucceedingFetcher implements [api.Fetcher] and fails
+// the first `failures` calls to FetchInfo, then succeeds.
+type failingThenSucceedingFetcher struct {
+	failures uint32
+	calls    atomic.Uint32
+	result   models.PublicIP
+}
+
+func (f *failingThenSucceedingFetcher) String() string      { return "test" }
+func (f *failingThenSucceedingFetcher) CanFetchAnyIP() bool { return true }
+func (f *failingThenSucceedingFetcher) Token() string       { return "" }
+
+func (f *failingThenSucceedingFetcher) FetchInfo(_ context.Context, _ netip.Addr) (
+	result models.PublicIP, err error,
+) {
+	call := f.calls.Add(1)
+	if call <= f.failures {
+		return models.PublicIP{}, errTest
+	}
+	return f.result, nil
+}
+
+type testLogger struct{}
+
+func (l *testLogger) Info(string)  {}
+func (l *testLogger) Warn(string)  {}
+func (l *testLogger) Error(string) {}
+
+func newTestLoop(fetcher api.Fetcher, initialBackoff time.Duration,
+	ipFilepath string,
+) *Loop {
+	logger := &testLogger{}
+	enabled := true
+	return &Loop{
+		settings: settings.PublicIP{
+			Enabled:    &enabled,
+			IPFilepath: &ipFilepath,
+		},
+		fetcher:             api.NewResilient([]api.Fetcher{fetcher}, logger),
+		logger:              logger,
+		puid:                os.Getuid(),
+		pgid:                os.Getgid(),
+		retryInitialBackoff: initialBackoff,
+		retryMaxBackoff:     20 * initialBackoff,
+		timeNow:             time.Now,
+	}
+}
+
+func Test_Loop_retries_after_failed_fetch(t *testing.T) {
+	t.Parallel()
+
+	fetcher := &failingThenSucceedingFetcher{
+		failures: 2,
+		result: models.PublicIP{
+			IP:      netip.AddrFrom4([4]byte{1, 2, 3, 4}),
+			Country: "Country",
+			Region:  "Region",
+			City:    "City",
+		},
+	}
+	const initialBackoff = 5 * time.Millisecond
+	ipFilepath := filepath.Join(t.TempDir(), "ip")
+	loop := newTestLoop(fetcher, initialBackoff, ipFilepath)
+
+	_, err := loop.Start(context.Background())
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		err := loop.Stop()
+		assert.NoError(t, err)
+	})
+
+	// The tunnel up trigger fetch fails and schedules a retry.
+	err = loop.RunOnce(context.Background())
+	require.ErrorIs(t, err, errTest)
+
+	// The first retry fails and the second retry succeeds.
+	const maxWait = 10 * time.Second
+	deadline := time.Now().Add(maxWait)
+	for !loop.GetData().IP.IsValid() && time.Now().Before(deadline) {
+		time.Sleep(initialBackoff)
+	}
+
+	assert.Equal(t, fetcher.result, loop.GetData())
+	assert.Equal(t, uint32(3), fetcher.calls.Load())
+	content, err := os.ReadFile(ipFilepath)
+	require.NoError(t, err)
+	assert.Equal(t, fetcher.result.IP.String(), string(content))
+}
+
+func Test_Loop_ClearData_stops_retries(t *testing.T) {
+	t.Parallel()
+
+	fetcher := &failingThenSucceedingFetcher{
+		failures: 1000, // always fail
+	}
+	const initialBackoff = 100 * time.Millisecond
+	ipFilepath := filepath.Join(t.TempDir(), "ip")
+	loop := newTestLoop(fetcher, initialBackoff, ipFilepath)
+
+	_, err := loop.Start(context.Background())
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		err := loop.Stop()
+		assert.NoError(t, err)
+	})
+
+	// The tunnel up trigger fetch fails and schedules a retry.
+	err = loop.RunOnce(context.Background())
+	require.ErrorIs(t, err, errTest)
+
+	// The VPN tunnel goes down before the first retry runs.
+	err = loop.ClearData()
+	require.NoError(t, err)
+
+	// Wait enough time for multiple retries to have run.
+	time.Sleep(5 * initialBackoff)
+	assert.Equal(t, uint32(1), fetcher.calls.Load())
+}