plugin/cache: add prefer_positive stale policy (#8378)

* plugin/cache: add prefer_positive stale policy Add an opt-in serve_stale_policy that prefers an eligible success-cache answer over denial-cache entries while serve_stale is enabled. Preserve the existing ncache-first behavior when the policy is absent. Also classify SOA-backed CNAME NODATA responses in the cache so incomplete answers cannot be selected as positive stale responses. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6 Signed-off-by: Nitin Nizhawan <nnizhawan@microsoft.com> * plugin/cache: retain last-known-good positive answers Keep an answering success-cache item reachable when a later NOERROR or referral response overwrites the visible cache key without answering the question. This lets prefer_positive survive empty responses, referrals, and additional-only data while leaving policy-off lookup behavior unchanged. Return the exact accepted verify refresh item instead of re-reading an ambiguous cache key, avoiding expired TTL wraparound for uncacheable replies. Add regression coverage for non-answer refreshes, NODATA, SERVFAIL, NOTIMP, stale-window expiry, and bounded verify reply shaping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6 Signed-off-by: Nitin Nizhawan <nnizhawan@microsoft.com> * plugin/cache: validate preferred stale answers Reject truncated, DNSSEC-expired, mismatched-class, unrelated ANY, and ambiguous CNAME refreshes before replacing a stale last-known-good answer. Precompute answer eligibility when cache items are created so prefer_positive hits avoid repeated CNAME walks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6 Signed-off-by: Nitin Nizhawan <nnizhawan@microsoft.com> --------- Signed-off-by: Nitin Nizhawan <nnizhawan@microsoft.com> Co-authored-by: Nitin Nizhawan <nnizhawan@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 25da81ab-92dd-4663-b480-efd6262090c6
2026-08-20 23:08:28 -04:00 · 2026-08-14 13:48:39 +05:30
parent 2eb7d16071
commit 87ccb6f90e
7 changed files with 658 additions and 52 deletions
--- a/plugin/cache/README.md
+++ b/plugin/cache/README.md
@@ -41,6 +41,7 @@ cache [TTL] [ZONES...] {
    denial CAPACITY [TTL] [MINTTL]
    prefetch AMOUNT [[DURATION] [PERCENTAGE%]]
    serve_stale [DURATION] [immediate [RESPONSE_TTL [FAILURE_RECHECK]] | verify [VERIFY_TIMEOUT [RESPONSE_TTL [FAILURE_RECHECK]]]]
+    serve_stale_policy prefer_positive
    servfail DURATION
    disable success|denial [ZONES...]
    keepttl
@@ -89,6 +90,16 @@ cache [TTL] [ZONES...] {
  intact. The default of `0` preserves the existing retry behavior. RFC 8767 recommends `30s` and says this
  value should not exceed 5 minutes. Examples: `serve_stale 1h immediate 30s 30s` and
  `serve_stale 1h verify 100ms 30s 30s`.
+* `serve_stale_policy` controls cache selection while `serve_stale` is enabled. The only supported policy is
+  `prefer_positive`. It checks the success cache before the denial cache and returns an eligible positive response
+  when it actually answers the question, even when a cached NXDOMAIN, NODATA, SERVFAIL, or NOTIMP response also
+  exists. The positive response must be unexpired or within the configured `serve_stale` duration.
+  The positive response is retained independently when a later NOERROR response does not answer the question (for example, an empty response
+  without SOA, a referral, or a response carrying data only in the additional section), so such a refresh cannot
+  destroy the last-known-good answer. A usable positive refresh replaces the retained answer.
+  The policy is disabled by default because it can mask legitimate record deletion or removal until the positive response exceeds
+  the stale duration or is evicted. In `immediate` mode, the stale positive response is returned first and the cache
+  refreshes in the background. In `verify` mode, only a refreshed positive answer replaces the stale response.
 * `servfail` cache SERVFAIL responses for **DURATION**.  Setting **DURATION** to 0 will disable caching of SERVFAIL
  responses.  If this option is not set, SERVFAIL responses will be cached for 5 seconds.  **DURATION** may not be
  greater than 5 minutes.
--- a/plugin/cache/cache.go
+++ b/plugin/cache/cache.go
@@ -46,6 +46,7 @@ type Cache struct {
 	staleUpTo          time.Duration
 	verifyStale        bool
 	verifyStaleTimeout time.Duration // 0 means wait for upstream until its own timeout (current default).
+	preferPositive     bool
 	staleTTL           time.Duration // TTL returned with stale responses; 0 preserves the legacy behavior.
 	staleRecheck       time.Duration // Delay after a failed refresh before another attempt; 0 preserves the legacy behavior.

@@ -124,6 +125,74 @@ func hasSOA(m *dns.Msg) bool {
 	return false
 }

+// cacheResponseType returns the response type used by the cache. Typify treats
+// any NOERROR response with a non-empty answer section as NoError, but RFC 2308
+// NODATA responses may contain a CNAME chain. Reclassify those responses when
+// an SOA provides the negative cache TTL.
+func cacheResponseType(m *dns.Msg, now time.Time) response.Type {
+	t, _ := response.Typify(m, now)
+	if t == response.NoError && hasSOA(m) && isNODATA(m) {
+		return response.NoData
+	}
+	return t
+}
+
+// answersQuestion reports whether a NOERROR response contains an answer to its
+// question. For types other than CNAME and ANY, the queried type must exist at
+// the terminal owner reached by following the CNAME chain from QNAME.
+func answersQuestion(m *dns.Msg) bool {
+	if m == nil || m.Rcode != dns.RcodeSuccess || len(m.Question) == 0 || len(m.Answer) == 0 {
+		return false
+	}
+	q := m.Question[0]
+	return answerHasType(m.Answer, q.Name, q.Qtype, q.Qclass)
+}
+
+func answerHasType(answer []dns.RR, name string, qtype, qclass uint16) bool {
+	if len(answer) == 0 {
+		return false
+	}
+	if qtype == dns.TypeANY {
+		for _, r := range answer {
+			h := r.Header()
+			if classMatches(h.Class, qclass) && strings.EqualFold(h.Name, name) {
+				return true
+			}
+		}
+		return false
+	}
+	if qtype == dns.TypeCNAME {
+		target, ok := uniqueCNAMETarget(answer, name, qclass)
+		return ok && target != ""
+	}
+	terminal, ok := canonicalName(answer, name, qclass)
+	if !ok {
+		return false
+	}
+	name = terminal
+	for _, r := range answer {
+		h := r.Header()
+		if h.Rrtype == qtype && classMatches(h.Class, qclass) && strings.EqualFold(h.Name, name) {
+			return true
+		}
+	}
+	return false
+}
+
+func classMatches(rrClass, qclass uint16) bool {
+	return qclass == dns.ClassANY || rrClass == qclass
+}
+
+// usableAnswer reports whether m is a complete, cache-valid positive response
+// that answers its question. It intentionally permits TTL-zero responses:
+// they are usable for the current client even though they are not retained.
+func usableAnswer(m *dns.Msg, now time.Time) bool {
+	if m == nil || m.Truncated || cacheResponseType(m, now) != response.NoError {
+		return false
+	}
+	return answersQuestion(m)
+}
+
 // isNODATA reports whether a NOERROR response with a non-empty answer section
 // does not answer the question. Following RFC 1034 section 3.6.2 and RFC 2308
 // sections 1 and 2.2, a query of any type other than CNAME (and ANY) is
@@ -139,39 +208,15 @@ func hasSOA(m *dns.Msg) bool {
 // toward re-querying upstream rather than caching a non-answer. An empty answer
 // section returns false so that legitimate positive responses carrying data
 // outside the answer section (for example the whoami plugin) remain cacheable.
-// ANY queries are excluded because any record answers them. Note: a bare DNAME
-// (RFC 6672) without its synthesized CNAME is treated as NODATA; standard
-// responses include the synthesized CNAME, which the chain walk follows.
+// An ANY query is answered only by a record at the queried owner and in the
+// requested class. Note: a bare DNAME (RFC 6672) without its synthesized CNAME
+// is treated as NODATA; standard responses include the synthesized CNAME, which
+// the chain walk follows.
 func isNODATA(m *dns.Msg) bool {
 	if len(m.Answer) == 0 {
 		return false
 	}
-	qtype := m.Question[0].Qtype
-	if qtype == dns.TypeANY {
-		return false
-	}
-	// A CNAME query is answered by the CNAME itself, so the chain is not
-	// followed; otherwise resolve it to the terminal owner name.
-	name := m.Question[0].Name
-	if qtype != dns.TypeCNAME {
-		terminal, ok := canonicalName(m.Answer, name)
-		if !ok {
-			// The CNAME chain is malformed (an owner with more than one
-			// distinct target, or a loop) and therefore has no well-defined
-			// QNAME per RFC 2181 section 10.1 and RFC 1034 section 3.6.2. Such
-			// a response cannot be shown to answer the question, so treat it as
-			// NODATA and (being SOA-less) leave it uncacheable.
-			return true
-		}
-		name = terminal
-	}
-	for _, r := range m.Answer {
-		h := r.Header()
-		if h.Rrtype == qtype && strings.EqualFold(h.Name, name) {
-			return false
-		}
-	}
-	return true
+	return !answersQuestion(m)
 }

 // canonicalName follows the owner-linked CNAME chain in answer starting at name
@@ -185,7 +230,7 @@ func isNODATA(m *dns.Msg) bool {
 // fail-closed: callers treat a malformed chain as a non-answer. Duplicate CNAME
 // records that name the same target are tolerated, since they still describe a
 // single canonical name.
-func canonicalName(answer []dns.RR, name string) (string, bool) {
+func canonicalName(answer []dns.RR, name string, qclass uint16) (string, bool) {
 	visited := nameSet{}
 	for {
 		if visited.contains(name) {
@@ -194,7 +239,7 @@ func canonicalName(answer []dns.RR, name string) (string, bool) {
 		}
 		visited.add(name)

-		target, ok := uniqueCNAMETarget(answer, name)
+		target, ok := uniqueCNAMETarget(answer, name, qclass)
 		if !ok {
 			// Owner has more than one distinct canonical name.
 			return name, false
@@ -212,10 +257,10 @@ func canonicalName(answer []dns.RR, name string) (string, bool) {
 // CNAME target, which violates RFC 2181 section 10.1. When owner has no CNAME the
 // returned target is empty and ok is true, marking a terminal owner. Duplicate
 // CNAME records naming the same target are tolerated.
-func uniqueCNAMETarget(answer []dns.RR, owner string) (target string, ok bool) {
+func uniqueCNAMETarget(answer []dns.RR, owner string, qclass uint16) (target string, ok bool) {
 	for _, r := range answer {
 		c, isCNAME := r.(*dns.CNAME)
-		if !isCNAME || !strings.EqualFold(c.Header().Name, owner) {
+		if !isCNAME || !classMatches(c.Header().Class, qclass) || !strings.EqualFold(c.Header().Name, owner) {
 			continue
 		}
 		if target != "" && !strings.EqualFold(target, c.Target) {
@@ -286,6 +331,8 @@ type ResponseWriter struct {
 	remoteAddr net.Addr

 	wildcardFunc func() string // function to retrieve wildcard name that synthesized the result.
+	lastResponse *dns.Msg      // last response after cache TTL and DNSSEC adjustments.
+	lastItem     *item         // cache item written by the last response, if cacheable.

 	pexcept []string // positive zone exceptions
 	nexcept []string // negative zone exceptions
@@ -364,7 +411,8 @@ func (w *ResponseWriter) Hijack() {
 // WriteMsg implements the dns.ResponseWriter interface.
 func (w *ResponseWriter) WriteMsg(res *dns.Msg) error {
 	res = res.Copy()
-	mt, _ := response.Typify(res, w.now().UTC())
+	w.lastItem = nil
+	mt := cacheResponseType(res, w.now().UTC())

 	// key returns empty string for anything we don't want to cache.
 	hasKey, key := key(w.state.Name(), res, mt, w.do, w.cd)
@@ -392,6 +440,7 @@ func (w *ResponseWriter) WriteMsg(res *dns.Msg) error {
 		// But retain AD bit if requester set the AD bit in the request, per RFC6840 5.7-5.8
 		res.AuthenticatedData = false
 	}
+	w.lastResponse = res.Copy()

 	if hasKey && duration > 0 {
 		if w.state.Match(res) {
@@ -424,11 +473,19 @@ func (w *ResponseWriter) set(m *dns.Msg, key uint64, mt response.Type, duration
 		if w.wildcardFunc != nil {
 			i.wildcard = w.wildcardFunc()
 		}
+		if w.preferPositive && !i.answering {
+			if previous, ok := w.pcache.Get(key); ok {
+				i.lastKnownGood = previous.answeringItem(w.state)
+			}
+		}
 		if w.pcache.Add(key, i) {
 			evictions.WithLabelValues(w.server, Success, w.zonesMetricLabel, w.viewMetricLabel).Inc()
 		}
-		// when pre-fetching, remove the negative cache entry if it exists
-		if w.prefetch {
+		w.lastItem = i
+		// A positive refresh is the newest state for this key. Under the
+		// prefer_positive policy, only remove the denial when this response
+		// actually answers the question.
+		if (!w.preferPositive && w.prefetch) || (w.preferPositive && i.answering) {
 			w.ncache.Remove(key)
 		}

@@ -444,6 +501,7 @@ func (w *ResponseWriter) set(m *dns.Msg, key uint64, mt response.Type, duration
 		if w.ncache.Add(key, i) {
 			evictions.WithLabelValues(w.server, Denial, w.zonesMetricLabel, w.viewMetricLabel).Inc()
 		}
+		w.lastItem = i

 	case response.OtherError:
 		// don't cache these
@@ -467,24 +525,43 @@ func (w *ResponseWriter) Write(buf []byte) (int, error) {
 type verifyStaleResponseWriter struct {
 	*ResponseWriter
 	refreshed bool // set to true if the last WriteMsg wrote to ResponseWriter, false otherwise.
+	response  *dns.Msg
+	item      *item
 }

 // newVerifyStaleResponseWriter returns a ResponseWriter to be used when verifying stale cache
 // entries. It only forwards matching, complete responses that successfully refresh the data
-// according to RFC8767, section 4 (response is NoError or NXDomain), and ignores other responses.
+// according to RFC8767, section 4 (response is NoError or NXDomain). With prefer_positive, only
+// a usable positive answer is forwarded; other matching responses are cached without being sent
+// to the client.
 func newVerifyStaleResponseWriter(w *ResponseWriter) *verifyStaleResponseWriter {
 	return &verifyStaleResponseWriter{
-		w,
-		false,
+		ResponseWriter: w,
 	}
 }

 // WriteMsg implements the dns.ResponseWriter interface.
 func (w *verifyStaleResponseWriter) WriteMsg(res *dns.Msg) error {
 	w.refreshed = false
+	w.response = nil
+	w.item = nil
 	if res == nil || res.Truncated || !w.state.Match(res) {
 		return nil
 	}
+	if w.preferPositive {
+		if usableAnswer(res, w.now().UTC()) {
+			w.refreshed = true
+			err := w.ResponseWriter.WriteMsg(res)
+			w.response = w.lastResponse
+			w.item = w.lastItem
+			return err
+		}
+		prefetch := w.prefetch
+		w.prefetch = true
+		err := w.ResponseWriter.WriteMsg(res)
+		w.prefetch = prefetch
+		return err
+	}
 	responseType, _ := response.Typify(res, w.now().UTC())
 	if responseType == response.OtherError || responseType == response.Meta || responseType == response.Update {
 		return nil
@@ -493,7 +570,10 @@ func (w *verifyStaleResponseWriter) WriteMsg(res *dns.Msg) error {
 		return nil
 	}
 	w.refreshed = true
-	return w.ResponseWriter.WriteMsg(res) // stores to the cache and sends to the client
+	err := w.ResponseWriter.WriteMsg(res) // stores to the cache and sends to the client
+	w.response = w.lastResponse
+	w.item = w.lastItem
+	return err
 }

 const (
--- a/plugin/cache/cache_test.go
+++ b/plugin/cache/cache_test.go
@@ -980,6 +980,9 @@ func TestServeFromStaleCacheFetchVerifyTimeoutFastUpstream(t *testing.T) {
 	if got := rec.Msg.Answer[0].Header().Ttl; got != 200 {
 		t.Errorf("expected fresh TTL=200, got %d", got)
 	}
+	if !rec.Msg.Authoritative {
+		t.Error("expected cached fresh response to preserve authoritative cache reply shaping")
+	}
 }

 func TestNegativeStaleMaskingPositiveCache(t *testing.T) {
@@ -1451,6 +1454,431 @@ func TestServfailDoesNotShadowPositiveCache(t *testing.T) {
 	}
 }

+func TestPreferPositiveCachePolicy(t *testing.T) {
+	c := New()
+	c.staleUpTo = time.Hour
+	now := time.Now()
+	c.now = func() time.Time { return now }
+
+	req := new(dns.Msg)
+	req.SetQuestion("example.org.", dns.TypeA)
+	state := request.Request{W: &test.ResponseWriter{}, Req: req}
+	k := hash(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled)
+
+	positive := new(dns.Msg)
+	positive.SetReply(req)
+	positive.Answer = []dns.RR{test.A("example.org. 60 IN A 192.0.2.1")}
+	c.pcache.Add(k, newItem(positive, now.Add(-2*time.Minute), time.Minute))
+
+	negative := new(dns.Msg)
+	negative.SetRcode(req, dns.RcodeNameError)
+	negative.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")}
+	c.ncache.Add(k, newItem(negative, now, 5*time.Minute))
+
+	if got := c.getIfNotStale(now, state, "test"); got == nil || got.Rcode != dns.RcodeNameError {
+		t.Fatalf("default policy should prefer ncache NXDOMAIN, got %+v", got)
+	}
+
+	c.preferPositive = true
+	if got := c.getIfNotStale(now, state, "test"); got == nil || got.Rcode != dns.RcodeSuccess {
+		t.Fatalf("prefer_positive should prefer eligible pcache answer, got %+v", got)
+	}
+}
+
+func TestPreferPositiveRejectsNonAnswer(t *testing.T) {
+	c := New()
+	c.staleUpTo = time.Hour
+	c.preferPositive = true
+	now := time.Now()
+
+	req := new(dns.Msg)
+	req.SetQuestion("alias.example.org.", dns.TypeA)
+	state := request.Request{W: &test.ResponseWriter{}, Req: req}
+	k := hash(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled)
+
+	incomplete := new(dns.Msg)
+	incomplete.SetReply(req)
+	incomplete.Answer = []dns.RR{test.CNAME("alias.example.org. 60 IN CNAME missing.example.org.")}
+	c.pcache.Add(k, newItem(incomplete, now, time.Minute))
+
+	negative := new(dns.Msg)
+	negative.SetRcode(req, dns.RcodeNameError)
+	negative.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")}
+	c.ncache.Add(k, newItem(negative, now, 5*time.Minute))
+
+	if got := c.getIfNotStale(now, state, "test"); got == nil || got.Rcode != dns.RcodeNameError {
+		t.Fatalf("non-answer pcache item must not shadow ncache, got %+v", got)
+	}
+}
+
+func TestAnswersQuestionStrictEligibility(t *testing.T) {
+	chAddress, err := dns.NewRR("cached.org. 60 CH A 192.0.2.20")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tests := []struct {
+		name   string
+		qtype  uint16
+		answer []dns.RR
+		want   bool
+	}{
+		{
+			name:   "direct A",
+			qtype:  dns.TypeA,
+			answer: []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")},
+			want:   true,
+		},
+		{
+			name:   "ANY matching owner",
+			qtype:  dns.TypeANY,
+			answer: []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")},
+			want:   true,
+		},
+		{
+			name:   "ANY unrelated owner",
+			qtype:  dns.TypeANY,
+			answer: []dns.RR{test.A("unrelated.org. 60 IN A 192.0.2.10")},
+			want:   false,
+		},
+		{
+			name:  "duplicate equivalent CNAME targets",
+			qtype: dns.TypeCNAME,
+			answer: []dns.RR{
+				test.CNAME("cached.org. 60 IN CNAME target.org."),
+				test.CNAME("cached.org. 60 IN CNAME target.org."),
+			},
+			want: true,
+		},
+		{
+			name:  "multiple CNAME targets",
+			qtype: dns.TypeCNAME,
+			answer: []dns.RR{
+				test.CNAME("cached.org. 60 IN CNAME first.org."),
+				test.CNAME("cached.org. 60 IN CNAME second.org."),
+			},
+			want: false,
+		},
+		{
+			name:   "wrong RR class",
+			qtype:  dns.TypeA,
+			answer: []dns.RR{chAddress},
+			want:   false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			req := new(dns.Msg)
+			req.SetQuestion("cached.org.", tc.qtype)
+			res := new(dns.Msg)
+			res.SetReply(req)
+			res.Answer = tc.answer
+
+			if got := answersQuestion(res); got != tc.want {
+				t.Fatalf("answersQuestion() = %t, want %t", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestPreferPositiveRetainsLKGAcrossNonAnswerSuccessRefreshes(t *testing.T) {
+	c := New()
+	c.staleUpTo = time.Hour
+	c.preferPositive = true
+	now := time.Now()
+	c.now = func() time.Time { return now }
+
+	req := new(dns.Msg)
+	req.SetQuestion("cached.org.", dns.TypeA)
+	state := request.Request{W: &test.ResponseWriter{}, Req: req}
+	writer := &ResponseWriter{Cache: c, state: state, prefetch: true}
+
+	positive := new(dns.Msg)
+	positive.SetReply(req)
+	positive.Answer = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")}
+	if err := writer.WriteMsg(positive); err != nil {
+		t.Fatal(err)
+	}
+
+	now = now.Add(2 * time.Minute)
+	refreshes := []*dns.Msg{
+		func() *dns.Msg {
+			m := new(dns.Msg)
+			m.SetReply(req)
+			return m
+		}(),
+		func() *dns.Msg {
+			m := new(dns.Msg)
+			m.SetReply(req)
+			m.Ns = []dns.RR{test.NS("example.org. 60 IN NS ns.example.org.")}
+			return m
+		}(),
+		func() *dns.Msg {
+			m := new(dns.Msg)
+			m.SetReply(req)
+			m.Ns = []dns.RR{test.NS("example.org. 60 IN NS ns.example.org.")}
+			m.Extra = []dns.RR{test.A("ns.example.org. 60 IN A 192.0.2.53")}
+			return m
+		}(),
+		func() *dns.Msg {
+			m := new(dns.Msg)
+			m.SetReply(req)
+			m.Extra = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.54")}
+			return m
+		}(),
+	}
+
+	for i, refresh := range refreshes {
+		if err := writer.WriteMsg(refresh); err != nil {
+			t.Fatal(err)
+		}
+		got := c.getIfNotStale(now, state, "test")
+		if got == nil || !got.answersQuestion(state) {
+			t.Fatalf("refresh %d lost last-known-good answer: %+v", i, got)
+		}
+		if address := got.Answer[0].(*dns.A).A.String(); address != "192.0.2.10" {
+			t.Fatalf("refresh %d returned %s, want 192.0.2.10", i, address)
+		}
+	}
+}
+
+func TestPreferPositiveDoesNotServeLKGOutsideStaleWindow(t *testing.T) {
+	c := New()
+	c.staleUpTo = time.Hour
+	c.preferPositive = true
+	now := time.Now()
+
+	req := new(dns.Msg)
+	req.SetQuestion("cached.org.", dns.TypeA)
+	state := request.Request{W: &test.ResponseWriter{}, Req: req}
+	k := hash(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled)
+
+	positive := new(dns.Msg)
+	positive.SetReply(req)
+	positive.Answer = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")}
+	lastKnownGood := newItem(positive, now.Add(-2*time.Hour), time.Minute)
+
+	empty := new(dns.Msg)
+	empty.SetReply(req)
+	current := newItem(empty, now, time.Minute)
+	current.lastKnownGood = lastKnownGood
+	c.pcache.Add(k, current)
+
+	negative := new(dns.Msg)
+	negative.SetRcode(req, dns.RcodeNameError)
+	negative.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")}
+	c.ncache.Add(k, newItem(negative, now, 5*time.Minute))
+
+	if got := c.getIfNotStale(now, state, "test"); got == nil || got.Rcode != dns.RcodeNameError {
+		t.Fatalf("expected current NXDOMAIN after LKG stale window, got %+v", got)
+	}
+}
+
+func TestPreferPositiveVerifyKeepsStaleOnNonAnswers(t *testing.T) {
+	tests := []struct {
+		name    string
+		backend plugin.Handler
+	}{
+		{name: "NXDOMAIN", backend: nxDomainBackend(300)},
+		{name: "NODATA", backend: plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+			m := new(dns.Msg)
+			m.SetReply(r)
+			m.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")}
+			return dns.RcodeSuccess, w.WriteMsg(m)
+		})},
+		{name: "SERVFAIL", backend: servFailBackend(300)},
+		{name: "NOTIMP", backend: plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+			m := new(dns.Msg)
+			m.SetRcode(r, dns.RcodeNotImplemented)
+			return dns.RcodeNotImplemented, w.WriteMsg(m)
+		})},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			c := New()
+			c.staleUpTo = time.Hour
+			c.verifyStale = true
+			c.preferPositive = true
+			c.Next = ttlBackend(60)
+
+			req := new(dns.Msg)
+			req.SetQuestion("cached.org.", dns.TypeA)
+			ctx := context.Background()
+			c.ServeDNS(ctx, &test.ResponseWriter{}, req)
+
+			c.now = func() time.Time { return time.Now().Add(2 * time.Minute) }
+			c.Next = tc.backend
+
+			rec := dnstest.NewRecorder(&test.ResponseWriter{})
+			ret, err := c.ServeDNS(ctx, rec, req.Copy())
+			if err != nil {
+				t.Fatal(err)
+			}
+			if ret != dns.RcodeSuccess || rec.Msg == nil || rec.Msg.Rcode != dns.RcodeSuccess {
+				t.Fatalf("expected stale positive response, got ret=%d msg=%+v", ret, rec.Msg)
+			}
+			if got := rec.Msg.Answer[0].Header().Ttl; got != 0 {
+				t.Fatalf("expected stale TTL 0, got %d", got)
+			}
+			if c.ncache.Len() != 1 {
+				t.Fatalf("expected verified %s to be retained in ncache, got %d entries", tc.name, c.ncache.Len())
+			}
+		})
+	}
+}
+
+func TestPreferPositiveVerifyRejectsInvalidFreshAnswers(t *testing.T) {
+	modes := []struct {
+		name    string
+		timeout time.Duration
+	}{
+		{name: "blocking"},
+		{name: "bounded", timeout: time.Second},
+	}
+	invalidResponses := []struct {
+		name  string
+		build func(*dns.Msg) *dns.Msg
+	}{
+		{
+			name: "expired RRSIG",
+			build: func(req *dns.Msg) *dns.Msg {
+				m := new(dns.Msg)
+				m.SetReply(req)
+				m.SetEdns0(4096, true)
+				m.Answer = []dns.RR{
+					test.A("cached.org. 60 IN A 192.0.2.20"),
+					test.RRSIG("cached.org. 60 IN RRSIG A 8 2 60 20160521031301 20160421031301 12051 cached.org. lAaEzB5teQLLKyDenatmyhca7blLRg9DoGNrhe3NReBZN5C5/pMQk8Jc u25hv2fW23/SLm5IC2zaDpp2Fzgm6Jf7e90/yLcwQPuE7JjS55WMF+HE LEh7Z6AEb+Iq4BWmNhUz6gPxD4d9eRMs7EAzk13o1NYi5/JhfL6IlaYy qkc="),
+				}
+				return m
+			},
+		},
+		{
+			name: "truncated",
+			build: func(req *dns.Msg) *dns.Msg {
+				m := new(dns.Msg)
+				m.SetReply(req)
+				m.Truncated = true
+				m.Answer = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.20")}
+				return m
+			},
+		},
+	}
+
+	for _, mode := range modes {
+		for _, invalid := range invalidResponses {
+			t.Run(mode.name+"/"+invalid.name, func(t *testing.T) {
+				c := New()
+				c.staleUpTo = time.Hour
+				c.verifyStale = true
+				c.verifyStaleTimeout = mode.timeout
+				c.preferPositive = true
+
+				now := time.Now().UTC()
+				c.now = func() time.Time { return now }
+				c.Next = plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+					m := new(dns.Msg)
+					m.SetReply(r)
+					m.Answer = []dns.RR{test.A("cached.org. 60 IN A 192.0.2.10")}
+					return dns.RcodeSuccess, w.WriteMsg(m)
+				})
+
+				req := new(dns.Msg)
+				req.SetQuestion("cached.org.", dns.TypeA)
+				req.SetEdns0(4096, true)
+				if _, err := c.ServeDNS(context.Background(), &test.ResponseWriter{}, req.Copy()); err != nil {
+					t.Fatal(err)
+				}
+
+				now = now.Add(2 * time.Minute)
+				c.Next = plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+					return dns.RcodeSuccess, w.WriteMsg(invalid.build(r))
+				})
+
+				rec := dnstest.NewRecorder(&test.ResponseWriter{})
+				ret, err := c.ServeDNS(context.Background(), rec, req.Copy())
+				if err != nil {
+					t.Fatal(err)
+				}
+				if ret != dns.RcodeSuccess || rec.Msg == nil || rec.Msg.Rcode != dns.RcodeSuccess {
+					t.Fatalf("expected stale positive response, got ret=%d msg=%+v", ret, rec.Msg)
+				}
+				if len(rec.Msg.Answer) == 0 {
+					t.Fatal("expected retained stale answer")
+				}
+				a, ok := rec.Msg.Answer[0].(*dns.A)
+				if !ok || a.A.String() != "192.0.2.10" {
+					t.Fatalf("expected retained 192.0.2.10, got %v", rec.Msg.Answer)
+				}
+				if got := a.Hdr.Ttl; got != 0 {
+					t.Fatalf("expected stale TTL 0, got %d", got)
+				}
+			})
+		}
+	}
+}
+
+func TestServeFromStaleCacheFetchVerifyTimeoutUncacheableResponse(t *testing.T) {
+	c := New()
+	c.staleUpTo = time.Hour
+	c.verifyStale = true
+	c.verifyStaleTimeout = time.Second
+	c.Next = ttlBackend(60)
+
+	req := new(dns.Msg)
+	req.SetQuestion("cached.org.", dns.TypeA)
+	c.ServeDNS(context.Background(), &test.ResponseWriter{}, req)
+	c.now = func() time.Time { return time.Now().Add(2 * time.Minute) }
+	c.Next = plugin.HandlerFunc(func(_ context.Context, w dns.ResponseWriter, r *dns.Msg) (int, error) {
+		m := new(dns.Msg)
+		m.SetRcode(r, dns.RcodeNameError)
+		return dns.RcodeNameError, w.WriteMsg(m)
+	})
+
+	rec := dnstest.NewRecorder(&test.ResponseWriter{})
+	ret, err := c.ServeDNS(context.Background(), rec, req.Copy())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if ret != dns.RcodeSuccess || rec.Msg == nil || rec.Msg.Rcode != dns.RcodeNameError {
+		t.Fatalf("expected direct uncacheable NXDOMAIN, got ret=%d msg=%+v", ret, rec.Msg)
+	}
+	for _, section := range [][]dns.RR{rec.Msg.Answer, rec.Msg.Ns, rec.Msg.Extra} {
+		for _, rr := range section {
+			if rr.Header().Ttl > uint32(maxTTL.Seconds()) {
+				t.Fatalf("unexpected wrapped TTL %d", rr.Header().Ttl)
+			}
+		}
+	}
+}
+
+func TestCNAMEWithSOAStoredAsNODATA(t *testing.T) {
+	c := New()
+	req := new(dns.Msg)
+	req.SetQuestion("alias.example.org.", dns.TypeA)
+	crr := &ResponseWriter{
+		Cache:    c,
+		state:    request.Request{Req: req},
+		prefetch: true,
+	}
+
+	res := new(dns.Msg)
+	res.SetReply(req)
+	res.Answer = []dns.RR{test.CNAME("alias.example.org. 300 IN CNAME missing.example.net.")}
+	res.Ns = []dns.RR{test.SOA("example.org. 300 IN SOA ns.example.org. hostmaster.example.org. 1 7200 3600 1209600 300")}
+
+	if err := crr.WriteMsg(res); err != nil {
+		t.Fatal(err)
+	}
+	if c.ncache.Len() != 1 {
+		t.Fatalf("expected NODATA in ncache, got %d entries", c.ncache.Len())
+	}
+	if c.pcache.Len() != 0 {
+		t.Fatalf("expected no positive cache entry, got %d", c.pcache.Len())
+	}
+}
+
 func TestServeFromStaleCacheFetchVerifyTimeoutMetadataIsolation(t *testing.T) {
 	c := New()
 	c.staleUpTo = time.Hour
--- a/plugin/cache/handler.go
+++ b/plugin/cache/handler.go
@@ -163,10 +163,10 @@ func (c *Cache) doRefresh(ctx context.Context, state request.Request, cw dns.Res

 // verifyWithTimeout runs the upstream verify in a background goroutine and races it
 // against verifyStaleTimeout. If the verify completes within the timeout and the
-// response is cacheable (NoError or NXDomain), the freshly cached entry is served
-// to the client and served is true. Otherwise served is false and the caller falls
-// through to serve stale; the goroutine continues to run and any successful response
-// will update the cache without writing to the (now-detached) client connection.
+// response is accepted by verifyStaleResponseWriter, the freshly cached entry is
+// served to the client and served is true. Otherwise served is false and the caller
+// falls through to serve stale; the goroutine continues to run and any cacheable
+// response updates the cache without writing to the detached client connection.
 func (c *Cache) verifyWithTimeout(ctx context.Context, state request.Request, w dns.ResponseWriter, cw *verifyStaleResponseWriter, r *dns.Msg, do, ad bool, i *item, failureRecheck time.Duration, now func() time.Time) (served bool, code int, err error) {
 	type result struct {
 		code int
@@ -191,17 +191,18 @@ func (c *Cache) verifyWithTimeout(ctx context.Context, state request.Request, w
 		if !cw.refreshed {
 			return false, 0, nil
 		}
-		fresh := c.exists(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled)
-		if fresh == nil {
-			// Should not happen: refreshed=true means the upstream response was cacheable.
+		if cw.response == nil {
 			return true, res.code, res.err
 		}
-		now := c.now()
-		if c.keepttl {
-			now = fresh.stored
+		response := cw.response
+		if cw.item != nil {
+			now := c.now()
+			if c.keepttl {
+				now = cw.item.stored
+			}
+			response = cw.item.toMsg(r, now, do, ad)
 		}
-		resp := fresh.toMsg(r, now, do, ad)
-		if err := w.WriteMsg(resp); err != nil {
+		if err := w.WriteMsg(response); err != nil {
 			return true, dns.RcodeServerFailure, err
 		}
 		return true, dns.RcodeSuccess, nil
@@ -227,6 +228,19 @@ func (c *Cache) getIfNotStale(now time.Time, state request.Request, server strin
 	k := hash(state.Name(), state.QType(), state.QClass(), state.Do(), state.Req.CheckingDisabled)
 	cacheRequests.WithLabelValues(server, c.zonesMetricLabel, c.viewMetricLabel).Inc()

+	if c.preferPositive && c.staleUpTo > 0 {
+		if i, ok := c.pcache.Get(k); ok {
+			i = i.answeringItem(state)
+			if i != nil {
+				ttl := i.ttl(now)
+				if ttl > 0 || -ttl < int(c.staleUpTo.Seconds()) {
+					cacheHits.WithLabelValues(server, Success, c.zonesMetricLabel, c.viewMetricLabel).Inc()
+					return i
+				}
+			}
+		}
+	}
+
 	if i, ok := c.ncache.Get(k); ok {
 		ttl := i.ttl(now)
 		if i.matches(state) && (ttl > 0 || (c.staleUpTo > 0 && -ttl < int(c.staleUpTo.Seconds()))) {
--- a/plugin/cache/item.go
+++ b/plugin/cache/item.go
@@ -22,6 +22,8 @@ type item struct {
 	Ns                 []dns.RR
 	Extra              []dns.RR
 	wildcard           string
+	answering          bool  // immutable result of validating that this item answers its question.
+	lastKnownGood      *item // answering item retained when a non-answer overwrites this success-cache key.

 	origTTL uint32
 	stored  time.Time
@@ -59,6 +61,7 @@ func newItem(m *dns.Msg, now time.Time, d time.Duration) *item {
 		j++
 	}
 	i.Extra = i.Extra[:j]
+	i.answering = answersQuestion(m)

 	i.origTTL = uint32(d.Seconds())
 	// Keep the monotonic clock reading so TTL expiry is unaffected by wall
@@ -119,6 +122,20 @@ func (i *item) matches(state request.Request) bool {
 	return false
 }

+func (i *item) answersQuestion(state request.Request) bool {
+	return i.answering && i.matches(state)
+}
+
+func (i *item) answeringItem(state request.Request) *item {
+	if i.answersQuestion(state) {
+		return i
+	}
+	if i.lastKnownGood != nil && i.lastKnownGood.answersQuestion(state) {
+		return i.lastKnownGood
+	}
+	return nil
+}
+
 func (i *item) beginRefresh(now time.Time, failureRecheck time.Duration) bool {
 	if failureRecheck > 0 {
 		if retryAfter := i.retryAfter.Load(); retryAfter != nil && now.Before(*retryAfter) {
--- a/plugin/cache/setup.go
+++ b/plugin/cache/setup.go
@@ -63,6 +63,8 @@ func cacheParse(c *caddy.Controller) (*Cache, error) {
 			}
 		}
 		origins := plugin.OriginsFromArgsOrServerBlock(args, c.ServerBlockKeys)
+		serveStaleConfigured := false
+		serveStalePolicyConfigured := false

 		// Refinements? In an extra block.
 		for c.NextBlock() {
@@ -171,6 +173,7 @@ func cacheParse(c *caddy.Controller) (*Cache, error) {
 				}

 			case "serve_stale":
+				serveStaleConfigured = true
 				args := c.RemainingArgs()
 				if len(args) > 5 {
 					return nil, c.ArgErr()
@@ -236,6 +239,21 @@ func cacheParse(c *caddy.Controller) (*Cache, error) {
 						}
 					}
 				}
+			case "serve_stale_policy":
+				if serveStalePolicyConfigured {
+					return nil, errors.New("serve_stale_policy can only be specified once")
+				}
+				serveStalePolicyConfigured = true
+				args := c.RemainingArgs()
+				if len(args) != 1 {
+					return nil, c.ArgErr()
+				}
+				switch strings.ToLower(args[0]) {
+				case "prefer_positive":
+					ca.preferPositive = true
+				default:
+					return nil, fmt.Errorf("invalid serve_stale_policy: %s", args[0])
+				}
 			case "servfail":
 				args := c.RemainingArgs()
 				if len(args) != 1 {
@@ -292,6 +310,9 @@ func cacheParse(c *caddy.Controller) (*Cache, error) {
 				return nil, c.ArgErr()
 			}
 		}
+		if serveStalePolicyConfigured && !serveStaleConfigured {
+			return nil, errors.New("serve_stale_policy requires serve_stale")
+		}

 		ca.Zones = origins
 		ca.zonesMetricLabel = strings.Join(origins, ",")
--- a/plugin/cache/setup_test.go
+++ b/plugin/cache/setup_test.go
@@ -226,6 +226,41 @@ func TestServeStaleFailureRecheck(t *testing.T) {
 	}
 }

+func TestServeStalePolicy(t *testing.T) {
+	tests := []struct {
+		input          string
+		shouldErr      bool
+		preferPositive bool
+	}{
+		{"serve_stale\nserve_stale_policy prefer_positive", false, true},
+		{"serve_stale_policy PREFER_POSITIVE\nserve_stale", false, true},
+		{"serve_stale", false, false},
+		// fails
+		{"serve_stale_policy prefer_positive", true, false},
+		{"serve_stale\nserve_stale_policy", true, false},
+		{"serve_stale\nserve_stale_policy prefer_positive extra", true, false},
+		{"serve_stale\nserve_stale_policy invalid", true, false},
+		{"serve_stale\nserve_stale_policy prefer_positive\nserve_stale_policy prefer_positive", true, false},
+	}
+	for i, test := range tests {
+		c := caddy.NewTestController("dns", fmt.Sprintf("cache {\n%s\n}", test.input))
+		ca, err := cacheParse(c)
+		if test.shouldErr && err == nil {
+			t.Errorf("Test %v: Expected error but found nil", i)
+			continue
+		} else if !test.shouldErr && err != nil {
+			t.Errorf("Test %v: Expected no error but found error: %v", i, err)
+			continue
+		}
+		if test.shouldErr {
+			continue
+		}
+		if ca.preferPositive != test.preferPositive {
+			t.Errorf("Test %v: Expected preferPositive %v but found %v", i, test.preferPositive, ca.preferPositive)
+		}
+	}
+}
+
 func TestServfail(t *testing.T) {
 	tests := []struct {
 		input     string