net/dns/resolver: don't retry an upstream REFUSED or SERVFAIL over TCP (#20829)

A TCP retry to the server that just answered can only return the same
rcode, and when that server accepted the connection and never answered,
the query ran the full 10s dnsQueryTimeout and the client got nothing.
An upstream rcode now ends the UDP/TCP race when it arrives, and the
client gets it once every resolver has had udpRaceTimeout to answer
instead of waiting for all of them to report. tcpQueryTimeout is also
set on the conn, since a context cannot interrupt a blocked read, and a
new dns_query_fwd_udp_read_ctx_done counter separates query teardown
from upstream read errors.

Fixes #20826

Signed-off-by: Brendan Creane <bcreane@gmail.com>

net/dns/resolver/forwarder.go

@@ -106,12 +106,22 @@ const (
 	// udpRaceTimeout is the timeout after which we will start a DNS query
 	// over TCP while waiting for the UDP query to complete.
 	udpRaceTimeout = 2 * time.Second
-
-	// tcpQueryTimeout is the timeout for a DNS query performed over TCP.
-	// It matches the default 5sec timeout of the 'dig' utility.
-	tcpQueryTimeout = 5 * time.Second
 )
 
+// tcpQueryTimeout is the timeout for a DNS query performed over TCP.
+// It matches the default 5sec timeout of the 'dig' utility.
+//
+// It's a var only so tests can shorten it.
+var tcpQueryTimeout = 5 * time.Second
+
+// rcodeHoldGrace is how long forwardWithDestChan holds an upstream REFUSED or
+// SERVFAIL after every resolver has started, in case a healthier one answers.
+// It matches udpRaceTimeout, since a resolver can't be treated as hung before
+// its UDP query has had a chance to escalate to TCP.
+//
+// It's a var only so tests can shorten it.
+var rcodeHoldGrace = udpRaceTimeout
+
 // txid identifies a DNS transaction.
 //
 // As the standard DNS Request ID is only 16 bits, we extend it:
@@ -747,6 +757,12 @@ func (f *forwarder) send(ctx context.Context, fq *forwardQuery, rr resolverAndDe
 	firstUDP := func(ctx context.Context) ([]byte, error) {
 		resp, err := f.sendUDP(ctx, fq, rr)
 		if err != nil {
+			if rcodeErr, ok := errors.AsType[rcodeResponseError](err); ok {
+				// An upstream REFUSED or SERVFAIL is a complete answer.
+				// Return it as the race's result so the race doesn't wait
+				// on a TCP retry. send converts it back to an error.
+				return rcodeErr.res, nil
+			}
 			return nil, err
 		}
 		if !truncatedFlagSet(resp) {
@@ -777,12 +793,9 @@ func (f *forwarder) send(ctx context.Context, fq *forwardQuery, rr resolverAndDe
 		return nil, truncatedResponseError{resp}
 	}
 	thenTCP := func(ctx context.Context) ([]byte, error) {
-		// If we're skipping the TCP fallback, then wait until the
-		// context is canceled and return that error (i.e. not
-		// returning anything).
+		// Report the disabled retry instead of waiting out the context.
 		if skipTCP {
-			<-ctx.Done()
-			return nil, ctx.Err()
+			return nil, errTCPRetryDisabled
 		}
 
 		return f.sendTCP(ctx, fq, rr)
@@ -799,6 +812,12 @@ func (f *forwarder) send(ctx context.Context, fq *forwardQuery, rr resolverAndDe
 	rh := race.New(timeout, firstUDP, thenTCP)
 	resp, err := rh.Start(ctx)
 	if err == nil {
+		// firstUDP returns an upstream REFUSED or SERVFAIL as a result.
+		// Convert it back to the soft error callers expect.
+		switch rcode := getRCode(resp); rcode {
+		case dns.RCodeServerFailure, dns.RCodeRefused:
+			return nil, rcodeResponseError{rcode, resp}
+		}
 		return resp, nil
 	}
 
@@ -839,6 +858,11 @@ var errRefused = errors.New("response code indicates refusal")
 var errServerFailure = errors.New("response code indicates server issue")
 var errTxIDMismatch = errors.New("txid doesn't match")
 
+// errTCPRetryDisabled is what the TCP arm of the race in send reports when TCP
+// retries are disabled, so that a failed UDP query returns at once instead of
+// waiting out its deadline.
+var errTCPRetryDisabled = errors.New("no TCP retry: disabled by envknob or control knob")
+
 func (f *forwarder) sendUDP(ctx context.Context, fq *forwardQuery, rr resolverAndDelay) (ret []byte, err error) {
 	ipp, ok := rr.name.IPPort()
 	if !ok {
@@ -883,6 +907,7 @@ func (f *forwarder) sendUDP(ctx context.Context, fq *forwardQuery, rr resolverAn
 		n, src, err = conn.ReadFromUDPAddrPort(out)
 		if err != nil {
 			if err := ctx.Err(); err != nil {
+				metricDNSFwdUDPReadCtxDone.Add(1)
 				return nil, err
 			}
 			if !neterror.PacketWasTruncated(err) {
@@ -1066,6 +1091,16 @@ func (f *forwarder) sendTCP(ctx context.Context, fq *forwardQuery, rr resolverAn
 	}
 	defer conn.Close()
 
+	// Canceling ctx can't interrupt reads blocked in the runtime, so the
+	// deadline has to go on the conn. ctx already carries
+	// min(caller's deadline, tcpQueryTimeout).
+	if deadline, ok := ctx.Deadline(); ok {
+		if err := conn.SetDeadline(deadline); err != nil {
+			// Best effort. The caller's ctx still bounds the read.
+			f.logf("sendTCP: setting conn deadline: %v", err)
+		}
+	}
+
 	fq.closeOnCtxDone.Add(conn)
 	defer fq.closeOnCtxDone.Remove(conn)
 
@@ -1345,7 +1380,13 @@ func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, respo
 		src:            query.addr,
 		closeOnCtxDone: new(closePool),
 	}
-	defer fq.closeOnCtxDone.Close()
+	defer func() {
+		// cancel must run first. The close wakes a blocked reader with
+		// net.ErrClosed, which sendUDP counts as an upstream read failure
+		// unless ctx is already done.
+		cancel()
+		fq.closeOnCtxDone.Close()
+	}()
 
 	if f.verboseFwd {
 		domainSha256 := sha256.Sum256([]byte(domain))
@@ -1355,6 +1396,7 @@ func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, respo
 
 	resc := make(chan []byte, 1) // it's fine buffered or not
 	errc := make(chan error, 1)  // it's fine buffered or not too
+	raceStart := time.Now()      // when the startDelay of each resolver below starts running
 	for i := range resolvers {
 		go func(rr *resolverAndDelay) {
 			if rr.startDelay > 0 {
@@ -1382,86 +1424,114 @@ func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, respo
 		}(&resolvers[i])
 	}
 
-	var firstErr error
+	// heldErr is the error deliverHeldResponse answers from, and the error
+	// returned when no response can be sent. A REFUSED never displaces another
+	// error and any other error displaces a REFUSED, so heldErr is a REFUSED
+	// only when every resolver refused.
+	var heldErr error
 	var numErr int
-	var sawNonRefused bool
+
+	noteErr := func(err error) {
+		numErr++
+		if heldErr == nil || (errors.Is(heldErr, errRefused) && !errors.Is(err, errRefused)) {
+			heldErr = err
+		}
+	}
+
+	// deliverSuccess gives the client an upstream's successful response.
+	deliverSuccess := func(v []byte) error {
+		select {
+		case <-ctx.Done():
+			metricDNSFwdErrorContext.Add(1)
+			return fmt.Errorf("waiting to send response: %w", ctx.Err())
+		case responseChan <- packet{v, query.family, query.addr}:
+			if f.verboseFwd {
+				f.logf("response(%d, %v, %d) = %d, nil", fq.txid, typ, len(domain), len(v))
+			}
+			metricDNSFwdSuccess.Add(1)
+			f.health.SetHealthy(dnsForwarderFailing)
+			return nil
+		}
+	}
+
+	// deliverHeldResponse gives the client the upstream response heldErr
+	// carries, or a synthesized SERVFAIL when it carries none. Like
+	// forwardWithDestChan, it either sends to responseChan and returns nil, or
+	// returns heldErr without sending.
+	deliverHeldResponse := func() error {
+		var res packet
+		if rcodeErr, ok := errors.AsType[rcodeResponseError](heldErr); ok {
+			res = packet{rcodeErr.res, query.family, query.addr}
+		} else {
+			r, err := servfailResponse(query)
+			if err != nil {
+				f.logf("building servfail response: %v", err)
+				return heldErr
+			}
+			res = r
+		}
+		select {
+		case <-ctx.Done():
+			metricDNSFwdErrorContext.Add(1)
+			metricDNSFwdErrorContextGotError.Add(1)
+			var resolverAddrs []string
+			for _, rr := range resolvers {
+				resolverAddrs = append(resolverAddrs, rr.name.Addr)
+			}
+			if f.acceptDNS {
+				f.health.SetUnhealthy(dnsForwarderFailing, health.Args{health.ArgDNSServers: strings.Join(resolverAddrs, ",")})
+			}
+		case responseChan <- res:
+			if f.verboseFwd {
+				f.logf("forwarder response(%d, %v, %d) = %d, %v", fq.txid, typ, len(domain), len(res.bs), heldErr)
+			}
+			return nil
+		}
+		return heldErr
+	}
+
+	// holdC stays nil, and a nil channel never fires, until an rcode error
+	// arrives with an upstream response to hold.
+	var holdC <-chan time.Time
+
 	for {
 		select {
 		case v := <-resc:
-			select {
-			case <-ctx.Done():
-				metricDNSFwdErrorContext.Add(1)
-				return fmt.Errorf("waiting to send response: %w", ctx.Err())
-			case responseChan <- packet{v, query.family, query.addr}:
-				if f.verboseFwd {
-					f.logf("response(%d, %v, %d) = %d, nil", fq.txid, typ, len(domain), len(v))
-				}
-				metricDNSFwdSuccess.Add(1)
-				f.health.SetHealthy(dnsForwarderFailing)
-				return nil
-			}
+			return deliverSuccess(v)
 		case err := <-errc:
-			if firstErr == nil {
-				firstErr = err
-			}
-			if !errors.Is(err, errRefused) {
-				sawNonRefused = true
-			}
-			numErr++
+			noteErr(err)
 			if numErr == len(resolvers) {
-				var res packet
-				if sawNonRefused {
-					// At least one server failed with SERVFAIL or a transport error
-					// (e.g. network failure, TxID mismatch, unsupported resolver type).
-					// All such errors map to SERVFAIL at the client level.
-					// Prefer returning the upstream SERVFAIL bytes from firstErr if
-					// available; otherwise synthesize a SERVFAIL response. Note the
-					// rcode guard: firstErr may be a REFUSED rcodeResponseError if it
-					// arrived before the SERVFAIL that set sawNonRefused.
-					if rcodeErr, ok := errors.AsType[rcodeResponseError](firstErr); ok && rcodeErr.rcode == dns.RCodeServerFailure {
-						res = packet{rcodeErr.res, query.family, query.addr}
-					} else {
-						r, err := servfailResponse(query)
-						if err != nil {
-							f.logf("building servfail response: %v", err)
-							return firstErr
-						}
-						res = r
-					}
-				} else {
-					// !sawNonRefused means every error was an rcodeResponseError with rcode REFUSED,
-					// so firstErr is guaranteed to wrap one.
-					rcodeErr, ok := errors.AsType[rcodeResponseError](firstErr)
-					if !ok {
-						f.logf("unexpected: all errors were REFUSED but firstErr is not rcodeResponseError: %v", firstErr)
-						return firstErr
-					}
-					res = packet{rcodeErr.res, query.family, query.addr}
-				}
-				select {
-				case <-ctx.Done():
-					metricDNSFwdErrorContext.Add(1)
-					metricDNSFwdErrorContextGotError.Add(1)
-					var resolverAddrs []string
-					for _, rr := range resolvers {
-						resolverAddrs = append(resolverAddrs, rr.name.Addr)
-					}
-					if f.acceptDNS {
-						f.health.SetUnhealthy(dnsForwarderFailing, health.Args{health.ArgDNSServers: strings.Join(resolverAddrs, ",")})
-					}
-				case responseChan <- res:
-					if f.verboseFwd {
-						f.logf("forwarder response(%d, %v, %d) = %d, %v", fq.txid, typ, len(domain), len(res.bs), firstErr)
-					}
-					return nil
-				}
-				return firstErr
+				return deliverHeldResponse()
 			}
+			// Hold this upstream's response in case nothing better arrives.
+			if _, ok := errors.AsType[rcodeResponseError](err); ok && holdC == nil {
+				timer := time.NewTimer(rcodeHoldDelay(resolvers, time.Since(raceStart)))
+				defer timer.Stop()
+				holdC = timer.C
+			}
+		case <-holdC:
+			// The select above may have picked the timer over an answer or an
+			// error that was ready at the same time. Take those first.
+			select {
+			case v := <-resc:
+				return deliverSuccess(v)
+			default:
+			}
+		drain:
+			for {
+				select {
+				case err := <-errc:
+					noteErr(err)
+				default:
+					break drain
+				}
+			}
+			return deliverHeldResponse()
 		case <-ctx.Done():
 			metricDNSFwdErrorContext.Add(1)
-			if firstErr != nil {
+			if heldErr != nil {
 				metricDNSFwdErrorContextGotError.Add(1)
-				return firstErr
+				return heldErr
 			}
 
 			// If we haven't got an error or a successful response,
@@ -1481,6 +1551,18 @@ func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, respo
 
 var initListenConfig func(_ *net.ListenConfig, _ *netmon.Monitor, tunName string) error
 
+// rcodeHoldDelay returns how much longer to hold an upstream REFUSED or
+// SERVFAIL before giving it to the client. The hold ends rcodeHoldGrace after
+// the last resolver's startDelay. elapsed is how long ago the resolvers were
+// started.
+func rcodeHoldDelay(resolvers []resolverAndDelay, elapsed time.Duration) time.Duration {
+	var lastStart time.Duration
+	for _, rr := range resolvers {
+		lastStart = max(lastStart, rr.startDelay)
+	}
+	return max(lastStart+rcodeHoldGrace-elapsed, 0)
+}
+
 // nameFromQuery extracts the normalized query name from bs.
 func nameFromQuery(bs []byte) (dnsname.FQDN, dns.Type, error) {
 	var parser dns.Parser

net/dns/resolver/forwarder_test.go

@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/binary"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -342,6 +343,14 @@ func BenchmarkNameFromQuery(b *testing.B) {
 type testDNSServerOptions struct {
 	SkipUDP bool
 	SkipTCP bool
+
+	// HangTCP accepts TCP connections and never answers them. With SkipTCP
+	// nothing listens, so the connect fails fast instead.
+	HangTCP bool
+
+	// ResponseDelay is how long the server waits before answering, on either
+	// transport.
+	ResponseDelay time.Duration
 }
 
 func runDNSServer(tb testing.TB, opts *testDNSServerOptions, response []byte, onRequest func(bool, []byte)) (port uint16) {
@@ -416,6 +425,10 @@ func runDNSServer(tb testing.TB, opts *testDNSServerOptions, response []byte, on
 		req = req[:n]
 		onRequest(true, req)
 
+		if opts != nil && opts.ResponseDelay > 0 {
+			time.Sleep(opts.ResponseDelay)
+		}
+
 		// Write response
 		if _, err := conn.Write(tcpResponse); err != nil {
 			logf("error writing response: %v", err)
@@ -426,12 +439,30 @@ func runDNSServer(tb testing.TB, opts *testDNSServerOptions, response []byte, on
 	var wg sync.WaitGroup
 
 	if opts == nil || !opts.SkipTCP {
+		hangTCP := opts != nil && opts.HangTCP
+		var hung []net.Conn
+		var hungMu sync.Mutex
+		if hangTCP {
+			tb.Cleanup(func() {
+				hungMu.Lock()
+				defer hungMu.Unlock()
+				for _, c := range hung {
+					c.Close()
+				}
+			})
+		}
 		wg.Go(func() {
 			for {
 				conn, err := tcpLn.Accept()
 				if err != nil {
 					return
 				}
+				if hangTCP {
+					hungMu.Lock()
+					hung = append(hung, conn)
+					hungMu.Unlock()
+					continue
+				}
 				go handleConn(conn)
 			}
 		})
@@ -439,6 +470,9 @@ func runDNSServer(tb testing.TB, opts *testDNSServerOptions, response []byte, on
 
 	handleUDP := func(addr netip.AddrPort, req []byte) {
 		onRequest(false, req)
+		if opts != nil && opts.ResponseDelay > 0 {
+			time.Sleep(opts.ResponseDelay)
+		}
 		if _, err := udpLn.WriteToUDPAddrPort(response, addr); err != nil {
 			logf("error writing response: %v", err)
 		}
@@ -503,11 +537,9 @@ func makeLargeResponse(tb testing.TB, domain string) (request, response []byte)
 	return
 }
 
-func runTestQuery(tb testing.TB, request []byte, modify func(*forwarder), ports ...uint16) ([]byte, error) {
-	return runTestQueryWithFamily(tb, request, "udp", modify, ports...)
-}
-
-func runTestQueryWithFamily(tb testing.TB, request []byte, family string, modify func(*forwarder), ports ...uint16) ([]byte, error) {
+// newTestForwarder returns a forwarder wired to a fresh event bus, netmon and
+// dialer. modify, if non-nil, runs on it before it is returned.
+func newTestForwarder(tb testing.TB, modify func(*forwarder)) *forwarder {
 	logf := tstest.WhileTestRunningLogger(tb)
 	bus := eventbustest.NewBus(tb)
 	netMon, err := netmon.New(bus, logf)
@@ -523,11 +555,26 @@ func runTestQueryWithFamily(tb testing.TB, request []byte, family string, modify
 	if modify != nil {
 		modify(fwd)
 	}
+	return fwd
+}
 
+func runTestQuery(tb testing.TB, request []byte, modify func(*forwarder), ports ...uint16) ([]byte, error) {
+	return runTestQueryWithFamily(tb, request, "udp", modify, ports...)
+}
+
+func runTestQueryWithFamily(tb testing.TB, request []byte, family string, modify func(*forwarder), ports ...uint16) ([]byte, error) {
 	resolvers := make([]resolverAndDelay, len(ports))
 	for i, port := range ports {
 		resolvers[i].name = &dnstype.Resolver{Addr: fmt.Sprintf("127.0.0.1:%d", port)}
 	}
+	return runTestQueryWithResolvers(tb, request, family, modify, resolvers...)
+}
+
+// runTestQueryWithResolvers is runTestQueryWithFamily for tests that need a
+// resolver the port-based helpers can't express, such as one with a non-IP
+// address or a startDelay.
+func runTestQueryWithResolvers(tb testing.TB, request []byte, family string, modify func(*forwarder), resolvers ...resolverAndDelay) ([]byte, error) {
+	fwd := newTestForwarder(tb, modify)
 
 	rpkt := packet{
 		bs:     request,
@@ -538,7 +585,7 @@ func runTestQueryWithFamily(tb testing.TB, request []byte, family string, modify
 	rchan := make(chan packet, 1)
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	tb.Cleanup(cancel)
-	err = fwd.forwardWithDestChan(ctx, rpkt, rchan, resolvers...)
+	err := fwd.forwardWithDestChan(ctx, rpkt, rchan, resolvers...)
 	select {
 	case res := <-rchan:
 		return res.bs, err
@@ -1279,6 +1326,353 @@ func TestForwarderIgnoresStrayDatagrams(t *testing.T) {
 	}
 }
 
+// TestForwarderRcodeNoTCPRetry checks that an upstream REFUSED or SERVFAIL
+// doesn't cost a TCP retry.
+//
+// REFUSED became a soft error in the fix for tailscale/tailscale#19053.
+func TestForwarderRcodeNoTCPRetry(t *testing.T) {
+	const domain = "rcode-no-retry.tailscale.com."
+	request := makeTestRequest(t, domain, dns.TypeA, 0)
+
+	for _, rcode := range []dns.RCode{dns.RCodeRefused, dns.RCodeServerFailure} {
+		t.Run(rcode.String(), func(t *testing.T) {
+			response := makeTestResponse(t, domain, rcode)
+
+			var sawTCPRequest atomic.Bool
+			port := runDNSServer(t, nil, response, func(isTCP bool, gotRequest []byte) {
+				if isTCP {
+					sawTCPRequest.Store(true)
+				}
+			})
+
+			resp, err := runTestQuery(t, request, beVerbose, port)
+			if err != nil {
+				t.Fatalf("runTestQuery: %v", err)
+			}
+			if !bytes.Equal(resp, response) {
+				t.Errorf("invalid response\ngot: %+v\nwant: %+v", resp, response)
+			}
+			if sawTCPRequest.Load() {
+				t.Error("upstream saw a TCP query; the rcode answer should not be retried")
+			}
+		})
+	}
+}
+
+// TestForwarderRaceLoserNoReadError checks that the losing resolver in a race
+// isn't counted in dns_query_fwd_udp_error_read when the query's teardown
+// closes its socket.
+func TestForwarderRaceLoserNoReadError(t *testing.T) {
+	const domain = "race-loser.tailscale.com."
+	request := makeTestRequest(t, domain, dns.TypeA, 0)
+	answer := makeTestResponse(t, domain, dns.RCodeSuccess, netip.MustParseAddr("127.0.0.1"))
+
+	answering := runDNSServer(t, nil, answer, func(isTCP bool, gotRequest []byte) {})
+	// Bound on both transports but answering on neither, so its read is still
+	// blocked when the other resolver's answer ends the query.
+	silent := runDNSServer(t, &testDNSServerOptions{SkipUDP: true, HangTCP: true},
+		answer, func(isTCP bool, gotRequest []byte) {})
+
+	const queries = 20
+	beforeRead := metricDNSFwdUDPErrorRead.Value()
+	beforeCtxDone := metricDNSFwdUDPReadCtxDone.Value()
+	for range queries {
+		resp, err := runTestQuery(t, request, nil, answering, silent)
+		if err != nil {
+			t.Fatalf("runTestQuery: %v", err)
+		}
+		if !bytes.Equal(resp, answer) {
+			t.Fatalf("invalid response\ngot: %+v\nwant: %+v", resp, answer)
+		}
+	}
+
+	// Wait for every losing read to wake up, whichever counter it lands in,
+	// before checking the counters.
+	if err := tstest.WaitFor(10*time.Second, func() error {
+		woke := (metricDNSFwdUDPReadCtxDone.Value() - beforeCtxDone) +
+			(metricDNSFwdUDPErrorRead.Value() - beforeRead)
+		if woke < queries {
+			return fmt.Errorf("only %d of %d losing reads have woken up", woke, queries)
+		}
+		return nil
+	}); err != nil {
+		t.Fatalf("waiting for the losing resolvers to finish: %v", err)
+	}
+	if got := metricDNSFwdUDPErrorRead.Value() - beforeRead; got != 0 {
+		t.Errorf("dns_query_fwd_udp_error_read advanced by %d over %d queries; want 0", got, queries)
+	}
+}
+
+// TestForwarderRcodeWithHungTCP checks that the client promptly gets the
+// REFUSED from an upstream that refuses over UDP and accepts TCP without ever
+// answering, for both udp and tcp queries.
+func TestForwarderRcodeWithHungTCP(t *testing.T) {
+	const domain = "refused-hung-tcp.tailscale.com."
+	request := makeTestRequest(t, domain, dns.TypeA, 0)
+	response := makeTestResponse(t, domain, dns.RCodeRefused)
+
+	port := runDNSServer(t, &testDNSServerOptions{HangTCP: true}, response, func(isTCP bool, gotRequest []byte) {})
+
+	// Longer than the query helper's context, so a regression that waits on
+	// the hung TCP arm fails as a context error.
+	tstest.Replace(t, &tcpQueryTimeout, 10*time.Second)
+
+	for _, family := range []string{"udp", "tcp"} {
+		t.Run(family, func(t *testing.T) {
+			// sendTCP bumps this before it dials.
+			beforeTCP := metricDNSFwdTCP.Value()
+			beforeCtx := metricDNSFwdErrorContext.Value()
+			resp, err := runTestQueryWithFamily(t, request, family, beVerbose, port)
+			if err != nil {
+				t.Fatalf("runTestQueryWithFamily: %v", err)
+			}
+			if !bytes.Equal(resp, response) {
+				t.Errorf("invalid response\ngot: %+v\nwant: %+v", resp, response)
+			}
+			if got := metricDNSFwdErrorContext.Value() - beforeCtx; got != 0 {
+				t.Errorf("dns_query_fwd_error_context advanced by %d; the REFUSED waited on the TCP arm", got)
+			}
+			// A tcp query races both transports from the start, so only the
+			// udp query must never send over TCP.
+			if got := metricDNSFwdTCP.Value() - beforeTCP; family == "udp" && got != 0 {
+				t.Errorf("dns_query_fwd_tcp advanced by %d; an rcode answer must not be retried over TCP", got)
+			}
+		})
+	}
+}
+
+// TestForwarderTCPRetriesDisabledDoesNotStall checks that the TCP arm reports
+// errTCPRetryDisabled right away rather than holding the resolver open until
+// the caller's context ends.
+func TestForwarderTCPRetriesDisabledDoesNotStall(t *testing.T) {
+	const domain = "no-tcp-retry.tailscale.com."
+	request := makeTestRequest(t, domain, dns.TypeA, 0)
+
+	fwd := newTestForwarder(t, setupForwarderWithTCPRetriesDisabled())
+
+	fq := &forwardQuery{
+		txid:           getTxID(request),
+		packet:         request,
+		family:         "udp",
+		closeOnCtxDone: new(closePool),
+	}
+	defer fq.closeOnCtxDone.Close()
+	// A non-IP resolver address makes the UDP arm fail immediately, so only
+	// the TCP arm can hold send open.
+	rr := resolverAndDelay{name: &dnstype.Resolver{Addr: "bogus-not-an-ip"}}
+
+	// Short, because a regression waits it out.
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	_, err := fwd.send(ctx, fq, rr)
+	if !errors.Is(err, errTCPRetryDisabled) {
+		t.Errorf("send err = %v; want it to wrap errTCPRetryDisabled", err)
+	}
+	if ctx.Err() != nil {
+		t.Error("send waited out the caller's context instead of reporting the disabled retry")
+	}
+}
+
+// TestForwarderRcodeHoldWithOutstandingResolver checks that an upstream REFUSED
+// reaches the client even though another resolver in the race never reports.
+func TestForwarderRcodeHoldWithOutstandingResolver(t *testing.T) {
+	const domain = "refused-with-hung-peer.tailscale.com."
+	request := makeTestRequest(t, domain, dns.TypeA, 0)
+	response := makeTestResponse(t, domain, dns.RCodeRefused)
+
+	// Shortened to keep the test fast.
+	tstest.Replace(t, &rcodeHoldGrace, 300*time.Millisecond)
+
+	refusingPort := runDNSServer(t, nil, response, func(isTCP bool, gotRequest []byte) {})
+	// Bound on both transports but answering on neither, so queries to it hang
+	// rather than failing fast.
+	hungPort := runDNSServer(t, &testDNSServerOptions{SkipUDP: true, HangTCP: true},
+		response, func(isTCP bool, gotRequest []byte) {})
+
+	// Bumped only when a forward runs out its context.
+	beforeCtx := metricDNSFwdErrorContext.Value()
+	resp, err := runTestQuery(t, request, beVerbose, refusingPort, hungPort)
+	if err != nil {
+		t.Fatalf("runTestQuery: %v", err)
+	}
+	if !bytes.Equal(resp, response) {
+		t.Errorf("invalid response\ngot: %+v\nwant: %+v", resp, response)
+	}
+	if got := metricDNSFwdErrorContext.Value() - beforeCtx; got != 0 {
+		t.Errorf("dns_query_fwd_error_context advanced by %d; the held REFUSED was not released before the context ended", got)
+	}
+}
+
+func TestRcodeHoldDelay(t *testing.T) {
+	res := func(delays ...time.Duration) []resolverAndDelay {
+		rr := make([]resolverAndDelay, len(delays))
+		for i, d := range delays {
+			rr[i] = resolverAndDelay{name: &dnstype.Resolver{Addr: "8.8.8.8:53"}, startDelay: d}
+		}
+		return rr
+	}
+	tests := []struct {
+		name      string
+		resolvers []resolverAndDelay
+		elapsed   time.Duration
+		want      time.Duration
+	}{
+		{"no delays", res(0, 0), 0, rcodeHoldGrace},
+		{"no delays, late rcode", res(0, 0), 50 * time.Millisecond, rcodeHoldGrace - 50*time.Millisecond},
+		{"waits out the largest delay", res(0, dohHeadStart), 0, dohHeadStart + rcodeHoldGrace},
+		{"delay already elapsed", res(0, dohHeadStart), dohHeadStart, rcodeHoldGrace},
+		{"grace already elapsed", res(0, dohHeadStart), dohHeadStart + rcodeHoldGrace + time.Second, 0},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := rcodeHoldDelay(tt.resolvers, tt.elapsed); got != tt.want {
+				t.Errorf("rcodeHoldDelay = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+// TestForwarderRcodeHoldWaitsForDelayedResolver checks that a fast REFUSED
+// doesn't cut off a resolver still inside its startDelay.
+func TestForwarderRcodeHoldWaitsForDelayedResolver(t *testing.T) {
+	const domain = "refused-then-delayed.tailscale.com."
+	request := makeTestRequest(t, domain, dns.TypeA, 0)
+	refused := makeTestResponse(t, domain, dns.RCodeRefused)
+	answer := makeTestResponse(t, domain, dns.RCodeSuccess, netip.MustParseAddr("127.0.0.1"))
+
+	refusingPort := runDNSServer(t, nil, refused, func(isTCP bool, gotRequest []byte) {})
+	answeringPort := runDNSServer(t, nil, answer, func(isTCP bool, gotRequest []byte) {})
+
+	resolvers := []resolverAndDelay{
+		{name: &dnstype.Resolver{Addr: fmt.Sprintf("127.0.0.1:%d", refusingPort)}},
+		{name: &dnstype.Resolver{Addr: fmt.Sprintf("127.0.0.1:%d", answeringPort)}, startDelay: dohHeadStart},
+	}
+
+	resp, err := runTestQueryWithResolvers(t, request, "udp", beVerbose, resolvers...)
+	if err != nil {
+		t.Fatalf("runTestQueryWithResolvers: %v", err)
+	}
+	// The answer can only exist if the hold outlasted the startDelay, so the
+	// bytes are the whole check.
+	if !bytes.Equal(resp, answer) {
+		t.Errorf("invalid response\ngot:  %+v\nwant: %+v", resp, answer)
+	}
+}
+
+// TestForwarderRcodeHoldWaitsForSlowResolver checks that a fast SERVFAIL from
+// one resolver doesn't beat a slower answer from another.
+func TestForwarderRcodeHoldWaitsForSlowResolver(t *testing.T) {
+	const domain = "servfail-then-slow-answer.tailscale.com."
+	request := makeTestRequest(t, domain, dns.TypeA, 0)
+	servfail := makeTestResponse(t, domain, dns.RCodeServerFailure)
+	answer := makeTestResponse(t, domain, dns.RCodeSuccess, netip.MustParseAddr("127.0.0.1"))
+
+	failingPort := runDNSServer(t, nil, servfail, func(isTCP bool, gotRequest []byte) {})
+	// 350ms is an ordinary cross-region latency, well inside rcodeHoldGrace.
+	slowPort := runDNSServer(t, &testDNSServerOptions{ResponseDelay: 350 * time.Millisecond},
+		answer, func(isTCP bool, gotRequest []byte) {})
+
+	resp, err := runTestQuery(t, request, beVerbose, failingPort, slowPort)
+	if err != nil {
+		t.Fatalf("runTestQuery: %v", err)
+	}
+	if !bytes.Equal(resp, answer) {
+		t.Errorf("invalid response\ngot:  %+v\nwant: %+v", resp, answer)
+	}
+}
+
+// TestForwarderHeldServfailPrefersUpstreamBytes checks that an upstream
+// SERVFAIL arriving after a REFUSED reaches the client as the upstream's own
+// response rather than a synthesized one.
+func TestForwarderHeldServfailPrefersUpstreamBytes(t *testing.T) {
+	const domain = "refused-then-servfail.tailscale.com."
+	request := makeTestRequest(t, domain, dns.TypeA, 0)
+	refused := makeTestResponse(t, domain, dns.RCodeRefused)
+
+	// RecursionAvailable is never set on a synthesized SERVFAIL, so the bytes
+	// tell the two apart.
+	b := dns.NewBuilder(nil, dns.Header{
+		Response:           true,
+		Authoritative:      true,
+		RecursionAvailable: true,
+		RCode:              dns.RCodeServerFailure,
+	})
+	b.StartQuestions()
+	b.Question(dns.Question{Name: dns.MustNewName(domain), Type: dns.TypeA, Class: dns.ClassINET})
+	servfail, err := b.Finish()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	refusingPort := runDNSServer(t, nil, refused, func(isTCP bool, gotRequest []byte) {})
+	failingPort := runDNSServer(t, &testDNSServerOptions{ResponseDelay: 100 * time.Millisecond},
+		servfail, func(isTCP bool, gotRequest []byte) {})
+
+	resp, err := runTestQuery(t, request, beVerbose, refusingPort, failingPort)
+	if err != nil {
+		t.Fatalf("runTestQuery: %v", err)
+	}
+	if !bytes.Equal(resp, servfail) {
+		t.Errorf("invalid response\ngot:  %+v\nwant: %+v", resp, servfail)
+	}
+}
+
+// TestSendTCPReadTimeout checks that tcpQueryTimeout bounds the response read
+// against an upstream that accepts the connection and never answers.
+func TestSendTCPReadTimeout(t *testing.T) {
+	const domain = "hung-tcp.tailscale.com."
+	request := makeTestRequest(t, domain, dns.TypeA, 0)
+
+	port := runDNSServer(t, &testDNSServerOptions{SkipUDP: true, HangTCP: true},
+		makeTestResponse(t, domain, dns.RCodeSuccess), func(isTCP bool, gotRequest []byte) {})
+
+	const shortTimeout = 250 * time.Millisecond
+	tstest.Replace(t, &tcpQueryTimeout, shortTimeout)
+
+	fwd := newTestForwarder(t, nil)
+
+	fq := &forwardQuery{
+		txid:           getTxID(request),
+		packet:         request,
+		family:         "udp",
+		closeOnCtxDone: new(closePool),
+	}
+	defer fq.closeOnCtxDone.Close()
+	rr := resolverAndDelay{name: &dnstype.Resolver{Addr: fmt.Sprintf("127.0.0.1:%d", port)}}
+
+	// A caller deadline well beyond tcpQueryTimeout, so the shorter one is
+	// the bound under test.
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	// In a goroutine because a regression hangs the read rather than
+	// returning late.
+	done := make(chan error, 1)
+	start := time.Now()
+	go func() {
+		_, err := fwd.sendTCP(ctx, fq, rr)
+		done <- err
+	}()
+	select {
+	case err := <-done:
+		t.Logf("sendTCP returned after %v: %v", time.Since(start), err)
+		// Either the conn deadline or the derived context's deadline may
+		// fire first, so match on the interface rather than on a sentinel.
+		var ne net.Error
+		if !errors.As(err, &ne) || !ne.Timeout() {
+			t.Errorf("sendTCP err = %v; want a deadline error", err)
+		}
+		// The caller's deadline is still live, so tcpQueryTimeout bounded
+		// the read.
+		if ctx.Err() != nil {
+			t.Errorf("the caller's context expired; tcpQueryTimeout (%v) was not the binding bound", shortTimeout)
+		}
+	case <-time.After(3 * time.Second):
+		t.Fatalf("sendTCP still running after 3s; tcpQueryTimeout (%v) is not bounding the read", shortTimeout)
+	}
+}
+
 // netstackUpstream is a resolver at a tailnet (CGNAT) address. In userspace
 // networking mode the host stack has no route to it; only netstack does.
 var netstackUpstream = netip.MustParseAddrPort("100.64.1.2:53")
@@ -1325,11 +1719,13 @@ func newNetstackDialer(tb testing.TB, netMon *netmon.Monitor, bus *eventbus.Bus,
 // case, where there is no tun device and so the host stack cannot reach the
 // tailnet.
 //
-// The two subtests send the same query to the same upstream and differ only in
-// the transport the forwarder picks; both must consult the dialer, so each
+// The two subtests send the same query and differ in the transport the
+// forwarder uses. Both transports must consult the dialer, so each subtest
 // asserts on the netstack dial hook. The UDP subtest also bounds elapsed by
 // udpRaceTimeout, since a regression that silently falls back to TCP still
-// produces the right bytes, just two seconds late.
+// produces the right bytes, just two seconds late. The TCP subtest's upstream
+// serves TCP only, so that a UDP answer can't win the race before the TCP arm
+// runs.
 //
 // See tailscale/tailscale#20314.
 func TestForwarderNetstackUpstream(t *testing.T) {
@@ -1339,8 +1735,12 @@ func TestForwarderNetstackUpstream(t *testing.T) {
 
 	for _, family := range []string{"tcp", "udp"} {
 		t.Run(family, func(t *testing.T) {
+			var opts *testDNSServerOptions
+			if family == "tcp" {
+				opts = &testDNSServerOptions{SkipUDP: true}
+			}
 			var sawUDP, sawTCP atomic.Bool
-			port := runDNSServer(t, nil, response, func(isTCP bool, gotRequest []byte) {
+			port := runDNSServer(t, opts, response, func(isTCP bool, gotRequest []byte) {
 				if isTCP {
 					sawTCP.Store(true)
 				} else {
@@ -1410,8 +1810,13 @@ func TestForwarderNetstackUpstream(t *testing.T) {
 					t.Errorf("query took %v (>= udpRaceTimeout %v): UDP never answered and the response came from the TCP fallback",
 						elapsed, udpRaceTimeout)
 				}
-			} else if dials.tcp.Load() == 0 {
-				t.Errorf("forwarder never dialed TCP via netstack")
+			} else {
+				if dials.tcp.Load() == 0 {
+					t.Errorf("forwarder never dialed TCP via netstack: the TCP path bypassed the dialer")
+				}
+				if !sawTCP.Load() {
+					t.Errorf("upstream never saw a TCP query")
+				}
 			}
 		})
 	}

net/dns/resolver/tsdns.go

@@ -1565,6 +1565,12 @@ var (
 	metricDNSFwdUDPErrorRead    = clientmetric.NewCounter("dns_query_fwd_udp_error_read")
 	metricDNSFwdUDPSuccess      = clientmetric.NewCounter("dns_query_fwd_udp_success")
 
+	// metricDNSFwdUDPReadCtxDone counts reads that ended because the query
+	// ended, either by losing a race or by running out its deadline. Those
+	// reads used to be counted as upstream read failures. Counting them here
+	// instead keeps them visible without blaming the upstream.
+	metricDNSFwdUDPReadCtxDone = clientmetric.NewCounter("dns_query_fwd_udp_read_ctx_done")
+
 	metricDNSFwdTCP             = clientmetric.NewCounter("dns_query_fwd_tcp")       // on entry
 	metricDNSFwdTCPWrote        = clientmetric.NewCounter("dns_query_fwd_tcp_wrote") // sent TCP packet
 	metricDNSFwdTCPErrorWrite   = clientmetric.NewCounter("dns_query_fwd_tcp_error_write")

tsnet/dns_split_test.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net"
 	"net/netip"
+	"sync"
 	"testing"
 	"time"
 
@@ -100,8 +101,130 @@ func TestSplitDNSToTailnetResolverUDP(t *testing.T) {
 	}
 }
 
-// serveOneAnswerDNS answers every A query on pc with addr, until pc is closed.
-func serveOneAnswerDNS(pc net.PacketConn, addr netip.Addr) {
+// TestRefusingUpstreamAnswersPromptly exercises the whole daemon DNS path,
+// from control pushing a resolver through dns.Manager to the forwarder. The
+// upstream answers REFUSED over UDP and accepts TCP without ever answering.
+// The client must get the REFUSED, not a stall that ends in no answer at all.
+//
+// See tailscale/tailscale#20826.
+func TestRefusingUpstreamAnswersPromptly(t *testing.T) {
+	ctx := t.Context()
+	controlURL, control := startControl(t)
+	s1, _, _ := startServer(t, ctx, controlURL, "s1")
+
+	// A host-loopback upstream, so this is the ordinary "system upstream" case
+	// rather than a tailnet address reached over netstack.
+	upstream := listenRefusingUpstream(t)
+
+	if !control.AddRawMapResponse(s1.lb.NodeKey(), &tailcfg.MapResponse{
+		DNSConfig: &tailcfg.DNSConfig{
+			Proxied:   true,
+			Resolvers: []*dnstype.Resolver{{Addr: upstream.String()}},
+		},
+	}) {
+		t.Fatal("AddRawMapResponse failed")
+	}
+
+	mgr := s1.Sys().DNSManager.Get()
+	query := mustDNSQuery(t, "refused.example.com.")
+	from := netip.MustParseAddrPort("127.0.0.1:12345")
+
+	// Wait for the pushed resolver to take effect. Until it lands there is no
+	// upstream at all, and the forwarder synthesizes its own SERVFAIL.
+	if err := tstest.WaitFor(60*time.Second, func() error {
+		ctx, cancel := context.WithTimeout(ctx, 3*time.Second)
+		defer cancel()
+		resp, err := mgr.Query(ctx, query, "udp", from)
+		if err != nil {
+			return err
+		}
+		if got := rcodeOf(t, resp); got != dns.RCodeRefused {
+			return fmt.Errorf("rcode = %v, want %v", got, dns.RCodeRefused)
+		}
+		return nil
+	}); err != nil {
+		t.Fatalf("waiting for the REFUSED upstream to take effect: %v", err)
+	}
+
+	for i := range 3 {
+		ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+		start := time.Now()
+		resp, err := mgr.Query(ctx, query, "udp", from)
+		elapsed := time.Since(start)
+		cancel()
+		if err != nil {
+			t.Fatalf("query %d: %v", i, err)
+		}
+		if got := rcodeOf(t, resp); got != dns.RCodeRefused {
+			t.Errorf("query %d: rcode = %v, want %v", i, got, dns.RCodeRefused)
+		}
+		// Logged only. A stall returns an error, which the check above catches.
+		t.Logf("query %d: %v", i, elapsed)
+	}
+}
+
+// listenRefusingUpstream starts a DNS upstream on host loopback that answers
+// REFUSED over UDP and accepts TCP connections without ever answering them. It
+// returns the address, and stops both listeners when the test ends.
+func listenRefusingUpstream(t *testing.T) netip.AddrPort {
+	t.Helper()
+	const tries = 25
+	var (
+		tcpLn net.Listener
+		pc    net.PacketConn
+	)
+	// Listen on TCP first, then bind UDP to the same port, retrying on a clash.
+	for range tries {
+		var err error
+		tcpLn, err = net.Listen("tcp", "127.0.0.1:0")
+		if err != nil {
+			t.Fatal(err)
+		}
+		pc, err = net.ListenPacket("udp", tcpLn.Addr().String())
+		if err == nil {
+			break
+		}
+		tcpLn.Close()
+		tcpLn = nil
+	}
+	if tcpLn == nil {
+		t.Skipf("failed to listen on the same port for TCP and UDP after %d tries", tries)
+	}
+
+	// Hold accepted conns open until the test ends, so the forwarder's TCP
+	// retry, if it makes one, connects and then waits rather than failing fast.
+	var (
+		heldMu sync.Mutex
+		held   []net.Conn
+	)
+	t.Cleanup(func() {
+		tcpLn.Close()
+		pc.Close()
+		heldMu.Lock()
+		defer heldMu.Unlock()
+		for _, c := range held {
+			c.Close()
+		}
+	})
+	go serveRefusedDNS(pc)
+	go func() {
+		for {
+			c, err := tcpLn.Accept()
+			if err != nil {
+				return
+			}
+			heldMu.Lock()
+			held = append(held, c)
+			heldMu.Unlock()
+		}
+	}()
+	return netip.MustParseAddrPort(pc.LocalAddr().String())
+}
+
+// serveDNS answers every query on pc with whatever respond builds from the
+// query's header and question, until pc is closed. Queries that don't parse
+// and responses that don't build are dropped.
+func serveDNS(pc net.PacketConn, respond func(hdr dns.Header, q dns.Question) ([]byte, error)) {
 	buf := make([]byte, 1500)
 	for {
 		n, from, err := pc.ReadFrom(buf)
@@ -117,6 +240,40 @@ func serveOneAnswerDNS(pc net.PacketConn, addr netip.Addr) {
 		if err != nil {
 			continue
 		}
+		resp, err := respond(hdr, q)
+		if err != nil {
+			continue
+		}
+		pc.WriteTo(resp, from)
+	}
+}
+
+// serveRefusedDNS answers every query on pc with REFUSED, until pc is closed.
+func serveRefusedDNS(pc net.PacketConn) {
+	serveDNS(pc, func(hdr dns.Header, q dns.Question) ([]byte, error) {
+		hdr.Response = true
+		hdr.RCode = dns.RCodeRefused
+		b := dns.NewBuilder(nil, hdr)
+		b.StartQuestions()
+		b.Question(q)
+		return b.Finish()
+	})
+}
+
+// rcodeOf returns the response code in resp.
+func rcodeOf(tb testing.TB, resp []byte) dns.RCode {
+	tb.Helper()
+	var p dns.Parser
+	h, err := p.Start(resp)
+	if err != nil {
+		tb.Fatalf("parsing response: %v", err)
+	}
+	return h.RCode
+}
+
+// serveOneAnswerDNS answers every A query on pc with addr, until pc is closed.
+func serveOneAnswerDNS(pc net.PacketConn, addr netip.Addr) {
+	serveDNS(pc, func(hdr dns.Header, q dns.Question) ([]byte, error) {
 		b := dns.NewBuilder(nil, dns.Header{ID: hdr.ID, Response: true})
 		b.StartQuestions()
 		b.Question(q)
@@ -126,12 +283,8 @@ func serveOneAnswerDNS(pc net.PacketConn, addr netip.Addr) {
 			Class: dns.ClassINET,
 			TTL:   300,
 		}, dns.AResource{A: addr.As4()})
-		resp, err := b.Finish()
-		if err != nil {
-			continue
-		}
-		pc.WriteTo(resp, from)
-	}
+		return b.Finish()
+	})
 }
 
 // mustDNSQuery builds an A query for domain.

tstest/natlab/vmtest/dns_test.go

@@ -788,3 +788,89 @@ func TestBareNameNotHijackedByPeer(t *testing.T) {
 		t.Fatalf("%v\nclient resolver state:\n%s", err, out)
 	}
 }
+
+// TestForwardedRefusedRcode checks that quad-100 hands the client an upstream
+// REFUSED instead of retrying it over TCP. vnet's second DNS server refuses
+// [vnet.SplitDNSRefusedName] and drops TCP to its port, so such a retry never
+// gets an answer.
+//
+// Lookups go through getent, so they reach the forwarder over UDP as a
+// client's would. The direct DNS backend keeps systemd-resolved from retrying
+// the REFUSED over TCP itself. Such a retry would bump the counter the test
+// asserts on. The test asserts on tailscaled's counters rather than on elapsed
+// time, since an emulated guest takes seconds just to start a process.
+//
+// See tailscale/tailscale#20826.
+func TestForwardedRefusedRcode(t *testing.T) {
+	fwdResolver := vnet.FakeSplitDNSIPv4()
+
+	env := vmtest.New(t,
+		vmtest.ControlDNS("tailnet.test", &tailcfg.DNSConfig{
+			// Proxied adds a nil-resolver route for the MagicDNS domain, so the
+			// route sets differ and quad-100 does the forwarding rather than
+			// handing fwdResolver to the OS.
+			Proxied: true,
+			Routes: map[string][]*dnstype.Resolver{
+				vnet.SplitDNSDomain: {{Addr: fwdResolver.String()}},
+			},
+		}))
+
+	lan := env.AddNetwork("2.1.1.1", "192.168.1.1/24", vnet.EasyNAT)
+	client := env.AddNode("client", lan,
+		vmtest.OS(vmtest.Ubuntu2404),
+		vmtest.WithDNSMode(vmtest.DNSDirect))
+
+	env.Start()
+
+	// libc must be sending to quad-100, not to fwdResolver directly, or the
+	// forwarder isn't in the path at all.
+	assertResolverState(t, env, client,
+		[]string{"100.100.100.100"},
+		[]string{fwdResolver.String()})
+
+	// A name the same server answers normally, as a positive control. Retrying
+	// it also waits for the DNS config to be applied.
+	if err := tstest.WaitFor(30*time.Second, func() error {
+		out, err := env.SSHExec(client, "getent ahostsv4 "+vnet.SplitDNSName)
+		if err != nil {
+			return fmt.Errorf("getent ahostsv4 %s: %v (%s)", vnet.SplitDNSName, err, strings.TrimSpace(out))
+		}
+		if !strings.Contains(out, vnet.SplitDNSAddr) {
+			return fmt.Errorf("getent ahostsv4 %s = %q, want it to contain %s",
+				vnet.SplitDNSName, strings.TrimSpace(out), vnet.SplitDNSAddr)
+		}
+		return nil
+	}); err != nil {
+		out, _ := env.SSHExec(client, "tailscale dns status")
+		t.Fatalf("%v\nclient DNS state:\n%s", err, out)
+	}
+
+	const (
+		tcpMetric     = "dns_query_fwd_tcp"
+		refusedMetric = "dns_query_fwd_udp_error_refused"
+	)
+	before := env.ClientMetrics(client)
+	for _, m := range []string{tcpMetric, refusedMetric} {
+		if _, ok := before[m]; !ok {
+			t.Fatalf("client has no %s metric", m)
+		}
+	}
+
+	// A REFUSED yields no address, so this lookup is expected to fail. The
+	// counters below are the check.
+	out, err := env.SSHExec(client, "getent ahostsv4 "+vnet.SplitDNSRefusedName)
+	t.Logf("getent ahostsv4 %s: err=%v out=%q", vnet.SplitDNSRefusedName, err, strings.TrimSpace(out))
+	if err == nil {
+		t.Errorf("getent ahostsv4 %s unexpectedly succeeded (%q); the upstream refuses that name",
+			vnet.SplitDNSRefusedName, strings.TrimSpace(out))
+	}
+
+	after := env.ClientMetrics(client)
+	if got, want := after[refusedMetric].Value, before[refusedMetric].Value; got <= want {
+		t.Errorf("%s did not advance (%v): the upstream's REFUSED never reached the forwarder",
+			refusedMetric, got)
+	}
+	if got, want := after[tcpMetric].Value, before[tcpMetric].Value; got != want {
+		t.Errorf("%s advanced from %v to %v: the rcode answer was retried over TCP", tcpMetric, want, got)
+	}
+}

tstest/natlab/vnet/vip.go

@@ -49,6 +49,13 @@ const (
 	// [FakeSplitDNSIPv4], not by a route for that domain.
 	SplitDNSBareName = "bare-upstream-only"
 	SplitDNSBareAddr = "10.99.1.2"
+
+	// SplitDNSRefusedName is answered REFUSED by fakeSplitDNS instead of with an
+	// address, for tests that need an upstream rcode rather than a resolution
+	// failure. Nothing in vnet serves DNS over TCP, and TCP to a DNS VIP is
+	// dropped rather than refused, so a forwarder that retries this rcode over
+	// TCP waits on an answer that never comes.
+	SplitDNSRefusedName = "refused." + SplitDNSDomain
 )
 
 // splitDNSZone holds the names served only by fakeSplitDNS.

tstest/natlab/vnet/vnet.go

@@ -2572,6 +2572,10 @@ func (s *Server) createDNSResponse(pkt gopacket.Packet) ([]byte, error) {
 		}
 
 		if toSplitDNS {
+			if string(q.Name) == SplitDNSRefusedName {
+				response.ResponseCode = layers.DNSResponseCodeRefused
+				continue
+			}
 			// The secondary server serves only its own zone, and only A records.
 			if addr, ok := splitDNSZone[string(q.Name)]; ok && q.Type == layers.DNSTypeA {
 				response.ANCount++

tstest/natlab/vnet/vnet_test.go

@@ -106,6 +106,16 @@ func TestPacketSideEffects(t *testing.T) {
 						pktSubstr("IP="+SplitDNSBareAddr),
 					),
 				},
+				{
+					// An upstream rcode, for tests that need one. The answer
+					// section stays empty.
+					name: "split-dns-refuses-refused-name",
+					pkt:  mkDNSReqTo(FakeSplitDNSIPv4(), SplitDNSRefusedName),
+					check: all(
+						numPkts(1),
+						pktSubstr("ResponseCode=Query Refused"),
+					),
+				},
 				{
 					// The default DNS server must NOT answer the split zone;
 					// that's what makes the answer above meaningful.