From 858999ba4ce11ea3a96ab4633e2b59448f4f5f8d Mon Sep 17 00:00:00 2001 From: Aditya Nugraha Date: Mon, 3 Aug 2026 22:10:38 +0700 Subject: [PATCH] proxy: report optimistic cache refreshes An optimistic cache hit is answered from the cache right away while the expired entry is refreshed in a separate goroutine, which calls replyFromUpstream on a clone of the context. That clone, and with it the QueryStatistics of every exchange the refresh performed, is then dropped. The refresh never reaches Config.RequestHandler either, so a caller has no way to observe those exchanges at all. Collecting response times from DNSContext.QueryStatistics therefore only ever samples cache misses, and with the optimistic cache enabled the popular names, which are the ones kept warm, are never sampled. The resulting average is biased towards the rare names that upstreams resolve slower. Add an optional Config.OnOptimisticRefresh, called once a background refresh finishes, with the context that carries its statistics. It is reported whether or not the refresh succeeded, since the statistics describe the attempt either way. Wrapping the upstreams instead does not work: Upstream.Exchange takes no context, and ExchangeParallel and ExchangeAll copy the request per upstream, so nothing identifies the request an exchange belongs to. See https://github.com/AdguardTeam/AdGuardHome/issues/8435. --- proxy/config.go | 13 +++++ proxy/optimisticresolver.go | 8 +++ proxy/optimisticresolver_internal_test.go | 63 +++++++++++++++++++++++ proxy/proxycache.go | 8 +++ 4 files changed, 92 insertions(+) diff --git a/proxy/config.go b/proxy/config.go index a56a5043c..cba561d07 100644 --- a/proxy/config.go +++ b/proxy/config.go @@ -65,6 +65,19 @@ type Config struct { // from this handler, the proxy will not send any response to the client. RequestHandler Handler + // OnOptimisticRefresh is called, if not nil, once the optimistic cache has + // refreshed an expired entry in the background. dctx is the context of + // that refresh; its [DNSContext.QueryStatistics] describe the exchanges it + // performed. Implementations must not modify or retain dctx, and must not + // block. + // + // An optimistic cache hit is answered from the cache right away and the + // entry is refreshed in a separate goroutine, so the exchanges of that + // refresh never reach [Config.RequestHandler]. Without this callback there + // is no way to account for them, which biases any statistics collected from + // [DNSContext.QueryStatistics] towards cache misses. + OnOptimisticRefresh func(dctx *DNSContext) + // UpstreamConfig is a general set of DNS servers to forward requests to. UpstreamConfig *UpstreamConfig diff --git a/proxy/optimisticresolver.go b/proxy/optimisticresolver.go index 011ade64d..fcdcea2c1 100644 --- a/proxy/optimisticresolver.go +++ b/proxy/optimisticresolver.go @@ -19,6 +19,10 @@ type cachingResolver interface { // cacheResp caches the response from dctx. cacheResp(dctx *DNSContext) + + // reportRefresh reports that dctx has been resolved in the background, so + // that the exchanges it performed can be accounted for. + reportRefresh(dctx *DNSContext) } // type check @@ -62,6 +66,10 @@ func (s *optimisticResolver) resolveOnce(dctx *DNSContext, key []byte, l *slog.L l.Debug("resolving request for optimistic cache", slogutil.KeyError, err) } + // Report the refresh even when it failed, since its statistics describe the + // attempt either way. + s.cr.reportRefresh(dctx) + if ok { s.cr.cacheResp(dctx) } diff --git a/proxy/optimisticresolver_internal_test.go b/proxy/optimisticresolver_internal_test.go index 58cde8767..d2e98b1aa 100644 --- a/proxy/optimisticresolver_internal_test.go +++ b/proxy/optimisticresolver_internal_test.go @@ -15,6 +15,7 @@ import ( type testCachingResolver struct { onReplyFromUpstream func(dctx *DNSContext) (ok bool, err error) onCacheResp func(dctx *DNSContext) + onReportRefresh func(dctx *DNSContext) } // replyFromUpstream implements the cachingResolver interface for @@ -28,6 +29,14 @@ func (tcr *testCachingResolver) cacheResp(dctx *DNSContext) { tcr.onCacheResp(dctx) } +// reportRefresh implements the cachingResolver interface for +// *testCachingResolver. +func (tcr *testCachingResolver) reportRefresh(dctx *DNSContext) { + if tcr.onReportRefresh != nil { + tcr.onReportRefresh(dctx) + } +} + func TestOptimisticResolver_ResolveOnce(t *testing.T) { in, out := make(chan unit), make(chan unit) var timesResolved, timesSet int @@ -113,3 +122,57 @@ func TestOptimisticResolver_ResolveOnce_unsuccessful(t *testing.T) { assert.False(t, cached) }) } + +func TestOptimisticResolver_ResolveOnce_reportRefresh(t *testing.T) { + t.Parallel() + + testCases := []struct { + replyErr error + name string + replyOK bool + wantSet bool + }{{ + replyErr: nil, + name: "success", + replyOK: true, + wantSet: true, + }, { + replyErr: assert.AnError, + name: "failure", + replyOK: false, + wantSet: false, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var reported, set int + dctx := &DNSContext{} + + s := newOptimisticResolver(&testCachingResolver{ + onReplyFromUpstream: func(_ *DNSContext) (ok bool, err error) { + return tc.replyOK, tc.replyErr + }, + onCacheResp: func(_ *DNSContext) { set++ }, + onReportRefresh: func(got *DNSContext) { + assert.Same(t, dctx, got) + + reported++ + }, + }) + + s.resolveOnce(dctx, []byte("key"), slog.New(slog.DiscardHandler)) + + // The refresh is reported whether or not it succeeded, since its + // statistics describe the attempt either way. + assert.Equal(t, 1, reported) + + if tc.wantSet { + assert.Equal(t, 1, set) + } else { + assert.Equal(t, 0, set) + } + }) + } +} diff --git a/proxy/proxycache.go b/proxy/proxycache.go index f161d9b74..569b0c90d 100644 --- a/proxy/proxycache.go +++ b/proxy/proxycache.go @@ -76,6 +76,14 @@ func cloneIPNet(n *net.IPNet) (clone *net.IPNet) { } } +// reportRefresh implements the [cachingResolver] interface for *Proxy. It +// notifies [Config.OnOptimisticRefresh], if it is set. +func (p *Proxy) reportRefresh(d *DNSContext) { + if p.OnOptimisticRefresh != nil { + p.OnOptimisticRefresh(d) + } +} + // cacheResp stores the response from d in general or subnet cache. In case the // cache is present in d, it's used first. func (p *Proxy) cacheResp(d *DNSContext) {