Merge b3cad050a9 into b13042b071

internal/dnsforward/dnsforward.go

@@ -376,7 +376,7 @@ func (s *Server) Resolve(ctx context.Context, net, host string) (addr []netip.Ad
 const (
 	// ErrRDNSNoData is returned by [RDNSExchanger.Exchange] when the answer
 	// section of response is either NODATA or has no PTR records.
-	ErrRDNSNoData errors.Error = "no ptr data in response"
+	ErrRDNSNoData = rdns.ErrNoData
 
 	// ErrRDNSFailed is returned by [RDNSExchanger.Exchange] if the received
 	// response is not a NOERROR or NXDOMAIN.

internal/rdns/rdns.go

@@ -12,6 +12,16 @@ import (
 	"github.com/bluele/gcache"
 )
 
+// maxErrorCacheTTL is the maximum period during which failed lookups aren't
+// retried.  It prevents request floods while allowing recovery from temporary
+// resolver outages sooner than the regular cache TTL.
+const maxErrorCacheTTL = 1 * time.Minute
+
+// ErrNoData is returned by an [Exchanger] when a successful DNS response has
+// no PTR data.  It is cached for the regular cache TTL rather than treated as a
+// temporary resolver failure.
+const ErrNoData errors.Error = "no ptr data in response"
+
 // Interface processes rDNS queries.
 type Interface interface {
 	// Process makes rDNS request and returns domain name.  changed indicates
@@ -96,10 +106,15 @@ func (r *Default) Process(ctx context.Context, ip netip.Addr) (host string, chan
 	host, ttl, err := r.exchanger.Exchange(ctx, ip)
 	if err != nil {
 		r.logger.DebugContext(ctx, "resolving", "ip", ip, slogutil.KeyError, err)
+		if errors.Is(err, ErrNoData) {
+			ttl = max(ttl, r.cacheTTL)
+		} else {
+			ttl = min(r.cacheTTL, maxErrorCacheTTL)
+		}
+	} else {
+		ttl = max(ttl, r.cacheTTL)
 	}
 
-	ttl = max(ttl, r.cacheTTL)
-
 	item := &cacheItem{
 		expiry: time.Now().Add(ttl),
 		host:   host,

internal/rdns/rdns_internal_test.go

@@ -0,0 +1,119 @@
+package rdns
+
+import (
+	"context"
+	"net/netip"
+	"testing"
+	"time"
+
+	"github.com/AdguardTeam/golibs/errors"
+	"github.com/AdguardTeam/golibs/logutil/slogutil"
+	"github.com/AdguardTeam/golibs/testutil"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type testExchanger func(
+	ctx context.Context,
+	ip netip.Addr,
+) (host string, ttl time.Duration, err error)
+
+// Exchange implements the [Exchanger] interface for testExchanger.
+func (f testExchanger) Exchange(
+	ctx context.Context,
+	ip netip.Addr,
+) (host string, ttl time.Duration, err error) {
+	return f(ctx, ip)
+}
+
+func TestDefault_Process_errorRetry(t *testing.T) {
+	const shortCacheTTL = 30 * time.Second
+
+	testCases := []struct {
+		err      error
+		name     string
+		cacheTTL time.Duration
+		wantTTL  time.Duration
+	}{
+		{
+			name:     "temporary_failure",
+			err:      errors.Error("service unavailable"),
+			cacheTTL: time.Hour,
+			wantTTL:  maxErrorCacheTTL,
+		},
+		{
+			name:     "temporary_failure_short_cache",
+			err:      errors.Error("service unavailable"),
+			cacheTTL: shortCacheTTL,
+			wantTTL:  shortCacheTTL,
+		},
+		{
+			name:     "no_data",
+			err:      ErrNoData,
+			cacheTTL: time.Hour,
+			wantTTL:  time.Hour,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			var (
+				available bool
+				hits      int
+			)
+
+			wantHost := "router.example"
+			exchanger := testExchanger(func(
+				_ context.Context,
+				_ netip.Addr,
+			) (host string, ttl time.Duration, err error) {
+				hits++
+				if !available {
+					return "", 0, tc.err
+				}
+
+				return wantHost, tc.cacheTTL, nil
+			})
+
+			r := New(&Config{
+				Logger:    slogutil.NewDiscardLogger(),
+				Exchanger: exchanger,
+				CacheSize: 1,
+				CacheTTL:  tc.cacheTTL,
+			})
+
+			ctx := testutil.ContextWithTimeout(t, time.Second)
+			ip := netip.MustParseAddr("192.168.1.1")
+			started := time.Now()
+
+			got, changed := r.Process(ctx, ip)
+			finished := time.Now()
+			require.True(t, changed)
+			assert.Empty(t, got)
+			require.Equal(t, 1, hits)
+
+			val, err := r.cache.Get(ip)
+			require.NoError(t, err)
+
+			item := val.(*cacheItem)
+			assert.False(t, item.expiry.Before(started.Add(tc.wantTTL)))
+			assert.False(t, item.expiry.After(finished.Add(tc.wantTTL)))
+
+			// The failure is suppressed during the retry window.
+			got, changed = r.Process(ctx, ip)
+			assert.False(t, changed)
+			assert.Empty(t, got)
+			assert.Equal(t, 1, hits)
+
+			// Simulate the retry delay passing and the private resolver becoming
+			// ready after AdGuard Home startup.
+			item.expiry = time.Now().Add(-time.Second)
+			available = true
+
+			got, changed = r.Process(ctx, ip)
+			require.True(t, changed)
+			assert.Equal(t, wantHost, got)
+			assert.Equal(t, 2, hits)
+		})
+	}
+}