From 69d6803cb29b60439303d3dc2cf98037d43b195f Mon Sep 17 00:00:00 2001 From: yorah Date: Fri, 10 Jul 2026 00:33:28 +0200 Subject: [PATCH] fix(publicip): retry fetching information on failure --- internal/publicip/data.go | 4 + internal/publicip/loop.go | 137 ++++++++++++++++++++++---------- internal/publicip/loop_test.go | 139 +++++++++++++++++++++++++++++++++ 3 files changed, 240 insertions(+), 40 deletions(-) create mode 100644 internal/publicip/loop_test.go diff --git a/internal/publicip/data.go b/internal/publicip/data.go index 76c45d6f1..917238448 100644 --- a/internal/publicip/data.go +++ b/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{} diff --git a/internal/publicip/loop.go b/internal/publicip/loop.go index 6533aa256..b7324cf67 100644 --- a/internal/publicip/loop.go +++ b/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) { diff --git a/internal/publicip/loop_test.go b/internal/publicip/loop_test.go new file mode 100644 index 000000000..9ecf02363 --- /dev/null +++ b/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()) +}