From 49c40827f6b88b4c12751086aab529298587e265 Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Sat, 28 Nov 2020 18:07:29 -0300
Subject: [PATCH 01/13] Avoid iterating name twice when constructing fctx->info

This is a minor performance improvement, we store the result of the
first call to strlcat to use as an offset in the next call when
constructing fctx->info string.
---
 lib/dns/resolver.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 447d85062b..b0a0ec7820 100644
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -4850,9 +4850,10 @@ fctx_create(dns_resolver_t *res, const dns_name_t *name, dns_rdatatype_t type,
 	isc_result_t iresult;
 	isc_interval_t interval;
 	unsigned int findoptions = 0;
-	char buf[DNS_NAME_FORMATSIZE + DNS_RDATATYPE_FORMATSIZE];
+	char buf[DNS_NAME_FORMATSIZE + DNS_RDATATYPE_FORMATSIZE + 1];
 	char typebuf[DNS_RDATATYPE_FORMATSIZE];
 	isc_mem_t *mctx;
+	size_t p;
 
 	/*
 	 * Caller must be holding the lock for bucket number 'bucketnum'.
@@ -4879,8 +4880,8 @@ fctx_create(dns_resolver_t *res, const dns_name_t *name, dns_rdatatype_t type,
 	 */
 	dns_name_format(name, buf, sizeof(buf));
 	dns_rdatatype_format(type, typebuf, sizeof(typebuf));
-	strlcat(buf, "/", sizeof(buf));
-	strlcat(buf, typebuf, sizeof(buf));
+	p = strlcat(buf, "/", sizeof(buf));
+	strlcat(buf + p, typebuf, sizeof(buf));
 	fctx->info = isc_mem_strdup(mctx, buf);
 
 	FCTXTRACE("create");

From 74840ec50befe691261b46e4f919448587f993b7 Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Sat, 28 Nov 2020 18:10:35 -0300
Subject: [PATCH 02/13] Added dns_view_staleanswerenabled() function

Since it takes a couple lines of code to check whether stale answers
are enabled for a given view, code was extracted out to a proper
function.
---
 lib/dns/include/dns/view.h  |  9 +++++++++
 lib/dns/view.c              | 22 ++++++++++++++++++++++
 lib/dns/win32/libdns.def.in |  1 +
 lib/ns/query.c              | 16 +++-------------
 4 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h
index ff72c8b758..1da372a89a 100644
--- a/lib/dns/include/dns/view.h
+++ b/lib/dns/include/dns/view.h
@@ -1343,6 +1343,15 @@ dns_view_setviewrevert(dns_view_t *view);
  *\li	'view' to be valid.
  */
 
+bool
+dns_view_staleanswerenabled(dns_view_t *view);
+/*%<
+ * Check if stale answers are enabled for this view.
+ *
+ * Requires:
+ *\li	'view' to be valid.
+ */
+
 ISC_LANG_ENDDECLS
 
 #endif /* DNS_VIEW_H */
diff --git a/lib/dns/view.c b/lib/dns/view.c
index 42c17dc6da..3572cec04a 100644
--- a/lib/dns/view.c
+++ b/lib/dns/view.c
@@ -2518,3 +2518,25 @@ dns_view_setviewrevert(dns_view_t *view) {
 		dns_zt_setviewrevert(zonetable);
 	}
 }
+
+bool
+dns_view_staleanswerenabled(dns_view_t *view) {
+	uint32_t stale_ttl = 0;
+	bool result = false;
+
+	REQUIRE(DNS_VIEW_VALID(view));
+
+	if (dns_db_getservestalettl(view->cachedb, &stale_ttl) != ISC_R_SUCCESS)
+	{
+		return (false);
+	}
+	if (stale_ttl > 0) {
+		if (view->staleanswersok == dns_stale_answer_yes) {
+			result = true;
+		} else if (view->staleanswersok == dns_stale_answer_conf) {
+			result = view->staleanswersenable;
+		}
+	}
+
+	return (result);
+}
diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in
index c9b4e1fe80..917484d1ec 100644
--- a/lib/dns/win32/libdns.def.in
+++ b/lib/dns/win32/libdns.def.in
@@ -1147,6 +1147,7 @@ dns_view_setrootdelonly
 dns_view_setviewcommit
 dns_view_setviewrevert
 dns_view_simplefind
+dns_view_staleanswerenabled
 dns_view_thaw
 dns_view_untrust
 dns_view_weakattach
diff --git a/lib/ns/query.c b/lib/ns/query.c
index ae0a7bef4b..a8712f3a99 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -5581,7 +5581,6 @@ query_lookup(query_ctx_t *qctx) {
 	dns_clientinfo_t ci;
 	dns_name_t *rpzqname = NULL;
 	unsigned int dboptions;
-	dns_ttl_t stale_ttl = 0;
 	dns_ttl_t stale_refresh = 0;
 	bool dbfind_stale = false;
 
@@ -5644,18 +5643,9 @@ query_lookup(query_ctx_t *qctx) {
 
 	(void)dns_db_getservestalerefresh(qctx->client->view->cachedb,
 					  &stale_refresh);
-	(void)dns_db_getservestalettl(qctx->client->view->cachedb, &stale_ttl);
-	if (stale_refresh > 0) {
-		if (qctx->client->view->staleanswersok == dns_stale_answer_yes)
-		{
-			dboptions |= DNS_DBFIND_STALEENABLED;
-		} else if (qctx->client->view->staleanswersok ==
-			   dns_stale_answer_conf) {
-			if (qctx->client->view->staleanswersenable &&
-			    stale_ttl > 0) {
-				dboptions |= DNS_DBFIND_STALEENABLED;
-			}
-		}
+	if (stale_refresh > 0 &&
+	    dns_view_staleanswerenabled(qctx->client->view)) {
+		dboptions |= DNS_DBFIND_STALEENABLED;
 	}
 
 	result = dns_db_findext(qctx->db, rpzqname, qctx->version, qctx->type,

From 171a5b75423ad5b9bde9c3cecf869f985f1926eb Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Fri, 11 Dec 2020 14:10:31 -0300
Subject: [PATCH 03/13] Add stale-answer-client-timeout option

The general logic behind the addition of this new feature works as
folows:

When a client query arrives, the basic path (query.c / ns_query_recurse)
was to create a fetch, waiting for completion in fetch_callback.

With the introduction of stale-answer-client-timeout, a new event of
type DNS_EVENT_TRYSTALE may invoke fetch_callback, whenever stale
answers are enabled and the fetch took longer than
stale-answer-client-timeout to complete.

When an event of type DNS_EVENT_TRYSTALE triggers fetch_callback, we
must ensure that the folowing happens:

1. Setup a new query context with the sole purpose of looking up for
   stale RRset only data, for that matters a new flag was added
   'DNS_DBFIND_STALEONLY' used in database lookups.

    . If a stale RRset is found, mark the original client query as
      answered (with a new query attribute named NS_QUERYATTR_ANSWERED),
      so when the fetch completion event is received later, we avoid
      answering the client twice.

    . If a stale RRset is not found, cleanup and wait for the normal
      fetch completion event.

2. In ns_query_done, we must change this part:
	/*
	 * If we're recursing then just return; the query will
	 * resume when recursion ends.
	 */
	if (RECURSING(qctx->client)) {
		return (qctx->result);
	}

   To this:

	if (RECURSING(qctx->client) && !QUERY_STALEONLY(qctx->client)) {
		return (qctx->result);
	}

   Otherwise we would not proceed to answer the client if it happened
   that a stale answer was found when looking up for stale only data.

When an event of type DNS_EVENT_FETCHDONE triggers fetch_callback, we
proceed as before, resuming query, updating stats, etc, but a few
exceptions had to be added, most important of which are two:

1. Before answering the client (ns_client_send), check if the query
   wasn't already answered before.

2. Before detaching a client, e.g.
   isc_nmhandle_detach(&client->reqhandle), ensure that this is the
   fetch completion event, and not the one triggered due to
   stale-answer-client-timeout, so a correct call would be:
   if (!QUERY_STALEONLY(client)) {
        isc_nmhandle_detach(&client->reqhandle);
   }

Other than these notes, comments were added in code in attempt to make
these updates easier to follow.
---
 bin/named/config.c             |   3 +-
 bin/named/server.c             |   5 +
 lib/dns/include/dns/db.h       |  29 ++++-
 lib/dns/include/dns/events.h   |   1 +
 lib/dns/include/dns/resolver.h |   7 +-
 lib/dns/include/dns/view.h     |   1 +
 lib/dns/rbtdb.c                |   7 ++
 lib/dns/resolver.c             | 210 ++++++++++++++++++++++++++++++++-
 lib/isccfg/namedconf.c         |   1 +
 lib/ns/client.c                |   4 +-
 lib/ns/include/ns/query.h      |   1 +
 lib/ns/query.c                 | 136 +++++++++++++++++----
 12 files changed, 374 insertions(+), 31 deletions(-)

diff --git a/bin/named/config.c b/bin/named/config.c
index 77c0abaaa5..2850ff9960 100644
--- a/bin/named/config.c
+++ b/bin/named/config.c
@@ -194,9 +194,10 @@ options {\n\
 	servfail-ttl 1;\n\
 #	sortlist <none>\n\
 	stale-answer-enable false;\n\
-	stale-refresh-time 30; /* 30 seconds */\n\
+	stale-answer-client-timeout 1800; /* in milliseconds */\n\
 	stale-answer-ttl 30; /* 30 seconds */\n\
 	stale-cache-enable false;\n\
+	stale-refresh-time 30; /* 30 seconds */\n\
 	synth-from-dnssec no;\n\
 #	topology <none>\n\
 	transfer-format many-answers;\n\
diff --git a/bin/named/server.c b/bin/named/server.c
index 839438161c..6888333087 100644
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -4485,6 +4485,11 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 		view->staleanswersok = dns_stale_answer_conf;
 	}
 
+	obj = NULL;
+	result = named_config_get(maps, "stale-answer-client-timeout", &obj);
+	INSIST(result == ISC_R_SUCCESS);
+	view->staleanswerclienttimeout = cfg_obj_asuint32(obj);
+
 	obj = NULL;
 	result = named_config_get(maps, "stale-refresh-time", &obj);
 	INSIST(result == ISC_R_SUCCESS);
diff --git a/lib/dns/include/dns/db.h b/lib/dns/include/dns/db.h
index b79dcae0fa..1e01c43dfd 100644
--- a/lib/dns/include/dns/db.h
+++ b/lib/dns/include/dns/db.h
@@ -239,8 +239,35 @@ struct dns_dbonupdatelistener {
 #define DNS_DBFIND_FORCENSEC3	0x0080
 #define DNS_DBFIND_ADDITIONALOK 0x0100
 #define DNS_DBFIND_NOZONECUT	0x0200
-#define DNS_DBFIND_STALEOK	0x0400
+
+/*
+ * DNS_DBFIND_STALEOK: This flag is set when BIND fails to refresh a
+ * RRset due to timeout (resolver-query-timeout), its intent is to
+ * try to look for stale data in cache as a fallback, but only if
+ * stale answers are enabled in configuration.
+ *
+ * This flag is also used to activate stale-refresh-time window, since it
+ * is the only way the database knows that a resolution has failed.
+ */
+#define DNS_DBFIND_STALEOK 0x0400
+
+/*
+ * DNS_DBFIND_STALEENABLED: This flag is used as a hint to the database
+ * that it may use stale data. It is always set during query lookup if
+ * stale answers are enabled, but only effectively used during
+ * stale-refresh-time window. Also during this window, the resolver will
+ * not try to resolve the query, in other words no attempt to refresh the
+ * data in cache is made when the stale-refresh-time window is active.
+ */
 #define DNS_DBFIND_STALEENABLED 0x0800
+
+/*
+ * DNS_DBFIND_STALEONLY: This new introduced flag is used when we want
+ * stale data from the database, but not due to a failure in resolution,
+ * it also doesn't require stale-refresh-time window timer to be active.
+ * As long as there is a stale RRset available, it should be returned.
+ */
+#define DNS_DBFIND_STALEONLY 0x1000
 /*@}*/
 
 /*@{*/
diff --git a/lib/dns/include/dns/events.h b/lib/dns/include/dns/events.h
index 0e066310d1..50a8d01ac6 100644
--- a/lib/dns/include/dns/events.h
+++ b/lib/dns/include/dns/events.h
@@ -78,6 +78,7 @@
 #define DNS_EVENT_CATZDELZONE	     (ISC_EVENTCLASS_DNS + 56)
 #define DNS_EVENT_RPZUPDATED	     (ISC_EVENTCLASS_DNS + 57)
 #define DNS_EVENT_STARTUPDATE	     (ISC_EVENTCLASS_DNS + 58)
+#define DNS_EVENT_TRYSTALE	     (ISC_EVENTCLASS_DNS + 59)
 
 #define DNS_EVENT_FIRSTEVENT (ISC_EVENTCLASS_DNS + 0)
 #define DNS_EVENT_LASTEVENT  (ISC_EVENTCLASS_DNS + 65535)
diff --git a/lib/dns/include/dns/resolver.h b/lib/dns/include/dns/resolver.h
index b9b20e1577..07b94fdcd1 100644
--- a/lib/dns/include/dns/resolver.h
+++ b/lib/dns/include/dns/resolver.h
@@ -129,9 +129,10 @@ typedef enum { dns_quotatype_zone = 0, dns_quotatype_server } dns_quotatype_t;
 		    *   if possible. */
 
 /* Reserved in use by adb.c		0x00400000 */
-#define DNS_FETCHOPT_EDNSVERSIONSET   0x00800000
-#define DNS_FETCHOPT_EDNSVERSIONMASK  0xff000000
-#define DNS_FETCHOPT_EDNSVERSIONSHIFT 24
+#define DNS_FETCHOPT_EDNSVERSIONSET	0x00800000
+#define DNS_FETCHOPT_EDNSVERSIONMASK	0xff000000
+#define DNS_FETCHOPT_EDNSVERSIONSHIFT	24
+#define DNS_FETCHOPT_TRYSTALE_ONTIMEOUT 0x01000000
 
 /*
  * Upper bounds of class of query RTT (ms).  Corresponds to
diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h
index 1da372a89a..c0a4eb5d53 100644
--- a/lib/dns/include/dns/view.h
+++ b/lib/dns/include/dns/view.h
@@ -173,6 +173,7 @@ struct dns_view {
 	dns_stale_answer_t    staleanswersok;	  /* rndc setting */
 	bool		      staleanswersenable; /* named.conf setting
 						   * */
+	uint32_t	  staleanswerclienttimeout;
 	uint16_t	  nocookieudp;
 	uint16_t	  padding;
 	dns_acl_t *	  pad_acl;
diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c
index b9a46cea40..24442bd3ef 100644
--- a/lib/dns/rbtdb.c
+++ b/lib/dns/rbtdb.c
@@ -4562,6 +4562,13 @@ check_stale_header(dns_rbtnode_t *node, rdatasetheader_t *header,
 			 */
 			if ((search->options & DNS_DBFIND_STALEOK) != 0) {
 				header->last_refresh_fail_ts = search->now;
+			} else if ((search->options & DNS_DBFIND_STALEONLY) !=
+				   0) {
+				/*
+				 * We want stale RRset only, so we don't skip
+				 * it.
+				 */
+				return (false);
 			} else if ((search->options &
 				    DNS_DBFIND_STALEENABLED) != 0 &&
 				   search->now <
diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index b0a0ec7820..066db581ea 100644
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -307,7 +307,9 @@ struct fetchctx {
 	dns_rdataset_t nameservers;
 	atomic_uint_fast32_t attributes;
 	isc_timer_t *timer;
+	isc_timer_t *timer_try_stale;
 	isc_time_t expires;
+	isc_time_t expires_try_stale;
 	isc_interval_t interval;
 	dns_message_t *qmessage;
 	ISC_LIST(resquery_t) queries;
@@ -1135,6 +1137,16 @@ fctx_starttimer(fetchctx_t *fctx) {
 				NULL, true));
 }
 
+static inline isc_result_t
+fctx_starttimer_trystale(fetchctx_t *fctx) {
+	/*
+	 * Start the stale-answer-client-timeout timer for fctx.
+	 */
+
+	return (isc_timer_reset(fctx->timer_try_stale, isc_timertype_once,
+				&fctx->expires_try_stale, NULL, true));
+}
+
 static inline void
 fctx_stoptimer(fetchctx_t *fctx) {
 	isc_result_t result;
@@ -1153,6 +1165,22 @@ fctx_stoptimer(fetchctx_t *fctx) {
 	}
 }
 
+static inline void
+fctx_stoptimer_trystale(fetchctx_t *fctx) {
+	isc_result_t result;
+
+	if (fctx->timer_try_stale != NULL) {
+		result = isc_timer_reset(fctx->timer_try_stale,
+					 isc_timertype_inactive, NULL, NULL,
+					 true);
+		if (result != ISC_R_SUCCESS) {
+			UNEXPECTED_ERROR(__FILE__, __LINE__,
+					 "isc_timer_reset(): %s",
+					 isc_result_totext(result));
+		}
+	}
+}
+
 static inline isc_result_t
 fctx_startidletimer(fetchctx_t *fctx, isc_interval_t *interval) {
 	/*
@@ -1537,6 +1565,7 @@ fctx_stopqueries(fetchctx_t *fctx, bool no_response, bool age_untried) {
 	FCTXTRACE("stopqueries");
 	fctx_cancelqueries(fctx, no_response, age_untried);
 	fctx_stoptimer(fctx);
+	fctx_stoptimer_trystale(fctx);
 }
 
 static inline void
@@ -1697,6 +1726,16 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result, int line) {
 	     event = next_event) {
 		next_event = ISC_LIST_NEXT(event, ev_link);
 		ISC_LIST_UNLINK(fctx->events, event, ev_link);
+		if (event->ev_type == DNS_EVENT_TRYSTALE) {
+			/*
+			 * Not applicable to TRY STALE events, this function is
+			 * called when the fetch has either completed or timed
+			 * out due to resolver-query-timeout being reached.
+			 */
+			isc_task_detach((isc_task_t **)&event->ev_sender);
+			isc_event_free((isc_event_t **)&event);
+			continue;
+		}
 		task = event->ev_sender;
 		event->ev_sender = fctx;
 		event->vresult = fctx->vresult;
@@ -4182,7 +4221,13 @@ fctx_try(fetchctx_t *fctx, bool retrying, bool badcache) {
 	 */
 	if (fctx->minimized && !fctx->forwarding) {
 		unsigned int options = fctx->options;
-		options &= ~DNS_FETCHOPT_QMINIMIZE;
+		/*
+		 * Also clear DNS_FETCHOPT_TRYSTALE_ONTIMEOUT here, otherwise
+		 * every query minimization step will activate the try-stale
+		 * timer again.
+		 */
+		options &= ~(DNS_FETCHOPT_QMINIMIZE |
+			     DNS_FETCHOPT_TRYSTALE_ONTIMEOUT);
 
 		/*
 		 * Is another QNAME minimization fetch still running?
@@ -4222,6 +4267,7 @@ fctx_try(fetchctx_t *fctx, bool retrying, bool badcache) {
 		fctx_increference(fctx);
 		task = res->buckets[bucketnum].task;
 		fctx_stoptimer(fctx);
+		fctx_stoptimer_trystale(fctx);
 		result = dns_resolver_createfetch(
 			fctx->res, &fctx->qminname, fctx->qmintype,
 			&fctx->domain, &fctx->nameservers, NULL, NULL, 0,
@@ -4247,6 +4293,7 @@ fctx_try(fetchctx_t *fctx, bool retrying, bool badcache) {
 	}
 
 	fctx_increference(fctx);
+
 	result = fctx_query(fctx, addrinfo, fctx->options);
 	if (result != ISC_R_SUCCESS) {
 		fctx_done(fctx, result, __LINE__);
@@ -4504,6 +4551,9 @@ fctx_destroy(fetchctx_t *fctx) {
 	isc_counter_detach(&fctx->qc);
 	fcount_decr(fctx);
 	isc_timer_detach(&fctx->timer);
+	if (fctx->timer_try_stale != NULL) {
+		isc_timer_detach(&fctx->timer_try_stale);
+	}
 	dns_message_detach(&fctx->qmessage);
 	if (dns_name_countlabels(&fctx->domain) > 0) {
 		dns_name_free(&fctx->domain, fctx->mctx);
@@ -4579,6 +4629,57 @@ fctx_timeout(isc_task_t *task, isc_event_t *event) {
 	isc_event_free(&event);
 }
 
+/*
+ * Fetch event handlers called if stale answers are enabled
+ * (stale-answer-enabled) and the fetch took more than
+ * stale-answer-client-timeout to complete.
+ */
+static void
+fctx_timeout_try_stale(isc_task_t *task, isc_event_t *event) {
+	fetchctx_t *fctx = event->ev_arg;
+	dns_fetchevent_t *dns_event, *next_event;
+	isc_task_t *sender_task;
+	unsigned int count = 0;
+
+	REQUIRE(VALID_FCTX(fctx));
+
+	UNUSED(task);
+
+	FCTXTRACE("timeout_try_stale");
+
+	if (event->ev_type != ISC_TIMEREVENT_LIFE) {
+		return;
+	}
+
+	LOCK(&fctx->res->buckets[fctx->bucketnum].lock);
+
+	/*
+	 * Trigger events of type DNS_EVENT_TRYSTALE.
+	 */
+	for (dns_event = ISC_LIST_HEAD(fctx->events); dns_event != NULL;
+	     dns_event = next_event)
+	{
+		next_event = ISC_LIST_NEXT(dns_event, ev_link);
+
+		if (dns_event->ev_type != DNS_EVENT_TRYSTALE) {
+			continue;
+		}
+
+		ISC_LIST_UNLINK(fctx->events, dns_event, ev_link);
+		sender_task = dns_event->ev_sender;
+		dns_event->ev_sender = fctx;
+		dns_event->vresult = ISC_R_TIMEDOUT;
+		dns_event->result = ISC_R_TIMEDOUT;
+
+		isc_task_sendanddetach(&sender_task, ISC_EVENT_PTR(&dns_event));
+		count++;
+	}
+
+	UNLOCK(&fctx->res->buckets[fctx->bucketnum].lock);
+
+	isc_event_free(&event);
+}
+
 static void
 fctx_shutdown(fetchctx_t *fctx) {
 	isc_event_t *cevent;
@@ -4760,6 +4861,9 @@ fctx_start(isc_task_t *task, isc_event_t *event) {
 		 * All is well.  Start working on the fetch.
 		 */
 		result = fctx_starttimer(fctx);
+		if (result == ISC_R_SUCCESS && fctx->timer_try_stale != NULL) {
+			result = fctx_starttimer_trystale(fctx);
+		}
 		if (result != ISC_R_SUCCESS) {
 			fctx_done(fctx, result, __LINE__);
 		} else {
@@ -4826,6 +4930,34 @@ fctx_join(fetchctx_t *fctx, isc_task_t *task, const isc_sockaddr_t *client,
 	return (ISC_R_SUCCESS);
 }
 
+static inline void
+fctx_add_event(fetchctx_t *fctx, isc_task_t *task, const isc_sockaddr_t *client,
+	       dns_messageid_t id, isc_taskaction_t action, void *arg,
+	       dns_fetch_t *fetch, isc_eventtype_t event_type) {
+	isc_task_t *tclone;
+	dns_fetchevent_t *event;
+	/*
+	 * We store the task we're going to send this event to in the
+	 * sender field.  We'll make the fetch the sender when we actually
+	 * send the event.
+	 */
+	tclone = NULL;
+	isc_task_attach(task, &tclone);
+	event = (dns_fetchevent_t *)isc_event_allocate(fctx->res->mctx, tclone,
+						       event_type, action, arg,
+						       sizeof(*event));
+	event->result = DNS_R_SERVFAIL;
+	event->qtype = fctx->type;
+	event->db = NULL;
+	event->node = NULL;
+	event->rdataset = NULL;
+	event->sigrdataset = NULL;
+	event->fetch = fetch;
+	event->client = client;
+	event->id = id;
+	ISC_LIST_APPEND(fctx->events, event, ev_link);
+}
+
 static inline void
 log_ns_ttl(fetchctx_t *fctx, const char *where) {
 	char namebuf[DNS_NAME_FORMATSIZE];
@@ -4854,6 +4986,7 @@ fctx_create(dns_resolver_t *res, const dns_name_t *name, dns_rdatatype_t type,
 	char typebuf[DNS_RDATATYPE_FORMATSIZE];
 	isc_mem_t *mctx;
 	size_t p;
+	bool try_stale;
 
 	/*
 	 * Caller must be holding the lock for bucket number 'bucketnum'.
@@ -5078,6 +5211,29 @@ fctx_create(dns_resolver_t *res, const dns_name_t *name, dns_rdatatype_t type,
 		goto cleanup_qmessage;
 	}
 
+	try_stale = ((options & DNS_FETCHOPT_TRYSTALE_ONTIMEOUT) != 0);
+	if (try_stale) {
+		INSIST(res->view->staleanswerclienttimeout <=
+		       (res->query_timeout - 1000));
+		/*
+		 * Compute an expiration time after which stale data will
+		 * attempted to be served, if stale answers are enabled and
+		 * target RRset is available in cache.
+		 */
+		isc_interval_set(
+			&interval, res->view->staleanswerclienttimeout / 1000,
+			res->view->staleanswerclienttimeout % 1000 * 1000000);
+		iresult = isc_time_nowplusinterval(&fctx->expires_try_stale,
+						   &interval);
+		if (iresult != ISC_R_SUCCESS) {
+			UNEXPECTED_ERROR(__FILE__, __LINE__,
+					 "isc_time_nowplusinterval: %s",
+					 isc_result_totext(iresult));
+			result = ISC_R_UNEXPECTED;
+			goto cleanup_qmessage;
+		}
+	}
+
 	/*
 	 * Default retry interval initialization.  We set the interval now
 	 * mostly so it won't be uninitialized.  It will be set to the
@@ -5086,10 +5242,11 @@ fctx_create(dns_resolver_t *res, const dns_name_t *name, dns_rdatatype_t type,
 	isc_interval_set(&fctx->interval, 2, 0);
 
 	/*
-	 * Create an inactive timer.  It will be made active when the fetch
-	 * is actually started.
+	 * Create an inactive timer for resolver-query-timeout. It
+	 * will be made active when the fetch is actually started.
 	 */
 	fctx->timer = NULL;
+
 	iresult = isc_timer_create(res->timermgr, isc_timertype_inactive, NULL,
 				   NULL, res->buckets[bucketnum].task,
 				   fctx_timeout, fctx, &fctx->timer);
@@ -5100,6 +5257,26 @@ fctx_create(dns_resolver_t *res, const dns_name_t *name, dns_rdatatype_t type,
 		goto cleanup_qmessage;
 	}
 
+	/*
+	 * If stale answers are enabled, then create an inactive timer
+	 * for stale-answer-client-timeout. It will be made active when
+	 * the fetch is actually started.
+	 */
+	fctx->timer_try_stale = NULL;
+	if (try_stale) {
+		iresult = isc_timer_create(
+			res->timermgr, isc_timertype_inactive, NULL, NULL,
+			res->buckets[bucketnum].task, fctx_timeout_try_stale,
+			fctx, &fctx->timer_try_stale);
+		if (iresult != ISC_R_SUCCESS) {
+			UNEXPECTED_ERROR(__FILE__, __LINE__,
+					 "isc_timer_create: %s",
+					 isc_result_totext(iresult));
+			result = ISC_R_UNEXPECTED;
+			goto cleanup_qmessage;
+		}
+	}
+
 	/*
 	 * Attach to the view's cache and adb.
 	 */
@@ -5144,6 +5321,7 @@ cleanup_mctx:
 	dns_adb_detach(&fctx->adb);
 	dns_db_detach(&fctx->cache);
 	isc_timer_detach(&fctx->timer);
+	isc_timer_detach(&fctx->timer_try_stale);
 
 cleanup_qmessage:
 	dns_message_detach(&fctx->qmessage);
@@ -5357,6 +5535,23 @@ clone_results(fetchctx_t *fctx) {
 	for (event = ISC_LIST_NEXT(hevent, ev_link); event != NULL;
 	     event = ISC_LIST_NEXT(event, ev_link))
 	{
+		if (event->ev_type == DNS_EVENT_TRYSTALE) {
+			/*
+			 * We don't need to clone resulting data to this
+			 * type of event, as its associated callback is only
+			 * called when stale-answer-client-timeout triggers,
+			 * and the logic in there doesn't expect any result
+			 * as input, as it will itself lookup for stale data
+			 * in cache to use as result, if any is available.
+			 *
+			 * Also, if we reached this point, then the whole fetch
+			 * context is done, it will cancel timers, process
+			 * associated callbacks of type DNS_EVENT_FETCHDONE, and
+			 * silently remove/free events of type
+			 * DNS_EVENT_TRYSTALE.
+			 */
+			continue;
+		}
 		name = dns_fixedname_name(&event->foundname);
 		dns_name_copynf(hname, name);
 		event->result = hevent->result;
@@ -6126,6 +6321,7 @@ cache_name(fetchctx_t *fctx, dns_name_t *name, dns_message_t *message,
 	    (!need_validation)) {
 		have_answer = true;
 		event = ISC_LIST_HEAD(fctx->events);
+
 		if (event != NULL) {
 			adbp = &event->db;
 			aname = dns_fixedname_name(&event->foundname);
@@ -10815,6 +11011,14 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name,
 
 	result = fctx_join(fctx, task, client, id, action, arg, rdataset,
 			   sigrdataset, fetch);
+
+	if (result == ISC_R_SUCCESS &&
+	    ((options & DNS_FETCHOPT_TRYSTALE_ONTIMEOUT) != 0))
+	{
+		fctx_add_event(fctx, task, client, id, action, arg, fetch,
+			       DNS_EVENT_TRYSTALE);
+	}
+
 	if (new_fctx) {
 		if (result == ISC_R_SUCCESS) {
 			/*
diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c
index b7ed28d24e..6b7c6fbabe 100644
--- a/lib/isccfg/namedconf.c
+++ b/lib/isccfg/namedconf.c
@@ -2027,6 +2027,7 @@ static cfg_clausedef_t view_clauses[] = {
 	{ "servfail-ttl", &cfg_type_duration, 0 },
 	{ "sortlist", &cfg_type_bracketed_aml, 0 },
 	{ "stale-answer-enable", &cfg_type_boolean, 0 },
+	{ "stale-answer-client-timeout", &cfg_type_uint32, 0 },
 	{ "stale-answer-ttl", &cfg_type_duration, 0 },
 	{ "stale-cache-enable", &cfg_type_boolean, 0 },
 	{ "stale-refresh-time", &cfg_type_duration, 0 },
diff --git a/lib/ns/client.c b/lib/ns/client.c
index 0889688d49..7cc96efc99 100644
--- a/lib/ns/client.c
+++ b/lib/ns/client.c
@@ -879,7 +879,9 @@ ns_client_error(ns_client_t *client, isc_result_t result) {
 		}
 	}
 
-	ns_client_send(client);
+	if ((client->query.attributes & NS_QUERYATTR_ANSWERED) == 0) {
+		ns_client_send(client);
+	}
 }
 
 isc_result_t
diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h
index 011a8b4c13..55060a3a8c 100644
--- a/lib/ns/include/ns/query.h
+++ b/lib/ns/include/ns/query.h
@@ -117,6 +117,7 @@ struct ns_query {
 #define NS_QUERYATTR_DNS64EXCLUDE    0x08000
 #define NS_QUERYATTR_RRL_CHECKED     0x10000
 #define NS_QUERYATTR_REDIRECT	     0x20000
+#define NS_QUERYATTR_ANSWERED	     0x40000
 
 typedef struct query_ctx query_ctx_t;
 
diff --git a/lib/ns/query.c b/lib/ns/query.c
index a8712f3a99..9dcbc7ab7c 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -133,6 +133,12 @@
 
 #define REDIRECT(c) (((c)->query.attributes & NS_QUERYATTR_REDIRECT) != 0)
 
+/*% Was the query already answered due to stale-answer-client-timeout? */
+#define QUERY_ANSWERED(c) (((c)->query.attributes & NS_QUERYATTR_ANSWERED) != 0)
+
+/*% Does the query only wants to check for stale RRset? */
+#define QUERY_STALEONLY(c) (((c)->query.dboptions & DNS_DBFIND_STALEONLY) != 0)
+
 /*% Does the rdataset 'r' have an attached 'No QNAME Proof'? */
 #define NOQNAME(r) (((r)->attributes & DNS_RDATASETATTR_NOQNAME) != 0)
 
@@ -557,8 +563,13 @@ query_send(ns_client_t *client) {
 	}
 
 	inc_stats(client, counter);
-	ns_client_send(client);
-	isc_nmhandle_detach(&client->reqhandle);
+	if (!QUERY_ANSWERED(client)) {
+		ns_client_send(client);
+	}
+
+	if (!QUERY_STALEONLY(client)) {
+		isc_nmhandle_detach(&client->reqhandle);
+	}
 }
 
 static void
@@ -585,7 +596,10 @@ query_error(ns_client_t *client, isc_result_t result, int line) {
 	log_queryerror(client, result, line, loglevel);
 
 	ns_client_error(client, result);
-	isc_nmhandle_detach(&client->reqhandle);
+
+	if (!QUERY_STALEONLY(client)) {
+		isc_nmhandle_detach(&client->reqhandle);
+	}
 }
 
 static void
@@ -598,7 +612,10 @@ query_next(ns_client_t *client, isc_result_t result) {
 		inc_stats(client, ns_statscounter_failure);
 	}
 	ns_client_drop(client, result);
-	isc_nmhandle_detach(&client->reqhandle);
+
+	if (!QUERY_STALEONLY(client)) {
+		isc_nmhandle_detach(&client->reqhandle);
+	}
 }
 
 static inline void
@@ -5158,7 +5175,7 @@ qctx_freedata(query_ctx_t *qctx) {
 		dns_db_detach(&qctx->zdb);
 	}
 
-	if (qctx->event != NULL) {
+	if (qctx->event != NULL && !QUERY_STALEONLY(qctx->client)) {
 		free_devent(qctx->client, ISC_EVENT_PTR(&qctx->event),
 			    &qctx->event);
 	}
@@ -5583,6 +5600,7 @@ query_lookup(query_ctx_t *qctx) {
 	unsigned int dboptions;
 	dns_ttl_t stale_refresh = 0;
 	bool dbfind_stale = false;
+	bool stale_ok;
 
 	CCTRACE(ISC_LOG_DEBUG(3), "query_lookup");
 
@@ -5681,10 +5699,10 @@ query_lookup(query_ctx_t *qctx) {
 	 * answer, otherwise "fresh" answers are also treated as stale.
 	 */
 	dbfind_stale = ((dboptions & DNS_DBFIND_STALEOK) != 0);
-	if (dbfind_stale != 0 ||
-	    (((dboptions & DNS_DBFIND_STALEENABLED) != 0) &&
-	     STALE(qctx->rdataset)))
-	{
+	stale_ok = ((dboptions &
+		     (DNS_DBFIND_STALEENABLED | DNS_DBFIND_STALEONLY)) != 0);
+
+	if (dbfind_stale != 0 || (stale_ok && STALE(qctx->rdataset))) {
 		char namebuf[DNS_NAME_FORMATSIZE];
 		bool success;
 
@@ -5709,6 +5727,12 @@ query_lookup(query_ctx_t *qctx) {
 				      "%s resolver failure, stale answer %s",
 				      namebuf,
 				      success ? "used" : "unavailable");
+		} else if ((dboptions & DNS_DBFIND_STALEONLY) != 0) {
+			isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
+				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
+				      "%s client timeout, stale answer %s",
+				      namebuf,
+				      success ? "used" : "unavailable");
 		} else {
 			isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
 				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
@@ -5719,21 +5743,75 @@ query_lookup(query_ctx_t *qctx) {
 		}
 
 		if (!success) {
-			QUERY_ERROR(qctx, DNS_R_SERVFAIL);
-			return (ns_query_done(qctx));
+			/*
+			 * If DNS_DBFIND_STALEONLY is set then it means
+			 * stale-answer-client-timeout was triggered, in
+			 * that case we only want to check if a stale RRset is
+			 * available, if that's the case we promptly answer the
+			 * client with the stale data found. If a stale RRset is
+			 * not available then we must wait for the original
+			 * query to be resumed in order to build a proper
+			 * answer.
+			 */
+			if ((dboptions & DNS_DBFIND_STALEONLY) == 0) {
+				QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+				return (ns_query_done(qctx));
+			}
+
+			return (result);
 		}
 	}
-	return (query_gotanswer(qctx, result));
+
+	/*
+	 * If DNS_DBFIND_STALEONLY is disabled then we proceed as normal,
+	 * otherwise we only proceed with query_gotanswer if we
+	 * successfully found a stale RRset in cache.
+	 */
+	if (((dboptions & DNS_DBFIND_STALEONLY) == 0) ||
+	    result == ISC_R_SUCCESS || result == DNS_R_GLUE ||
+	    result == DNS_R_ZONECUT)
+	{
+		return (query_gotanswer(qctx, result));
+	}
 
 cleanup:
 	return (result);
 }
 
 /*
- * Event handler to resume processing a query after recursion.
- * If the query has timed out or been canceled or the system
- * is shutting down, clean up and exit; otherwise, call
- * query_resume() to continue the ongoing work.
+ * Create a new query context with the sole intent
+ * of looking up for a stale RRset in cache.
+ * If an entry is found, we mark the original query as
+ * answered, in order to avoid answering the query twice,
+ * when the original fetch finishes.
+ */
+static inline void
+query_lookup_staleonly(ns_client_t *client, dns_fetchevent_t *devent) {
+	query_ctx_t qctx;
+	isc_result_t result;
+
+	qctx_init(client, &devent, client->query.qtype, &qctx);
+	dns_db_attach(client->view->cachedb, &qctx.db);
+	client->query.dboptions |= DNS_DBFIND_STALEONLY;
+	result = query_lookup(&qctx);
+	if (result == ISC_R_SUCCESS) {
+		client->query.attributes |= NS_QUERYATTR_ANSWERED;
+	}
+	if (qctx.node != NULL) {
+		dns_db_detachnode(qctx.db, &qctx.node);
+	}
+	qctx_freedata(&qctx);
+	client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
+	qctx_destroy(&qctx);
+	isc_event_free(ISC_EVENT_PTR(&qctx.event));
+}
+
+/*
+ * Event handler to resume processing a query after recursion, or when a
+ * client timeout is triggered. If the query has timed out or been cancelled
+ * or the system is shutting down, clean up and exit. If a client timeout is
+ * triggered, see if we can respond with a stale answer from cache. Otherwise,
+ * call query_resume() to continue the ongoing work.
  */
 static void
 fetch_callback(isc_task_t *task, isc_event_t *event) {
@@ -5748,7 +5826,8 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
 
 	UNUSED(task);
 
-	REQUIRE(event->ev_type == DNS_EVENT_FETCHDONE);
+	REQUIRE(event->ev_type == DNS_EVENT_FETCHDONE ||
+		event->ev_type == DNS_EVENT_TRYSTALE);
 	client = devent->ev_arg;
 	REQUIRE(NS_CLIENT_VALID(client));
 	REQUIRE(task == client->task);
@@ -5756,6 +5835,11 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
 
 	CTRACE(ISC_LOG_DEBUG(3), "fetch_callback");
 
+	if (event->ev_type == DNS_EVENT_TRYSTALE) {
+		query_lookup_staleonly(client, devent);
+		return;
+	}
+
 	LOCK(&client->query.fetchlock);
 	if (client->query.fetch != NULL) {
 		/*
@@ -6076,6 +6160,10 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname,
 		peeraddr = &client->peeraddr;
 	}
 
+	if (dns_view_staleanswerenabled(client->view)) {
+		client->query.fetchoptions |= DNS_FETCHOPT_TRYSTALE_ONTIMEOUT;
+	}
+
 	isc_nmhandle_attach(client->handle, &client->fetchhandle);
 	result = dns_resolver_createfetch(
 		client->view->resolver, qname, qtype, qdomain, nameservers,
@@ -7678,7 +7766,9 @@ query_addanswer(query_ctx_t *qctx) {
 		query_filter64(qctx);
 		ns_client_putrdataset(qctx->client, &qctx->rdataset);
 	} else {
-		if (!qctx->is_zone && RECURSIONOK(qctx->client)) {
+		if (!qctx->is_zone && RECURSIONOK(qctx->client) &&
+		    !QUERY_STALEONLY(qctx->client))
+		{
 			query_prefetch(qctx->client, qctx->fname,
 				       qctx->rdataset);
 		}
@@ -7786,7 +7876,7 @@ query_respond(query_ctx_t *qctx) {
 	 * We shouldn't ever fail to add 'rdataset'
 	 * because it's already in the answer.
 	 */
-	INSIST(qctx->rdataset == NULL);
+	INSIST(qctx->rdataset == NULL || QUERY_ANSWERED(qctx->client));
 
 	query_addauth(qctx);
 
@@ -11271,7 +11361,7 @@ ns_query_done(query_ctx_t *qctx) {
 	 * If we're recursing then just return; the query will
 	 * resume when recursion ends.
 	 */
-	if (RECURSING(qctx->client)) {
+	if (RECURSING(qctx->client) && !QUERY_STALEONLY(qctx->client)) {
 		return (qctx->result);
 	}
 
@@ -11282,8 +11372,10 @@ ns_query_done(query_ctx_t *qctx) {
 	 * to the AA bit if the auth-nxdomain config option
 	 * says so, then render and send the response.
 	 */
-	query_setup_sortlist(qctx);
-	query_glueanswer(qctx);
+	if (!QUERY_ANSWERED(qctx->client)) {
+		query_setup_sortlist(qctx);
+		query_glueanswer(qctx);
+	}
 
 	if (qctx->client->message->rcode == dns_rcode_nxdomain &&
 	    qctx->view->auth_nxdomain)

From a12bf4b61b7a56f5d1f1823f5c3af490bdd6250d Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Fri, 11 Dec 2020 16:12:48 -0300
Subject: [PATCH 04/13] Adjusted serve-stale test

After the addition of stale-answer-client-timeout a test was broken due
to the following behavior expected by the test.

1. Prime cache data.example txt.
2. Disable authoritative server.
3. Send a query for data.example txt.
4. Recursive server will timeout and answer from cache with stale RRset.
5. Recursive server will activate stale-refresh-time due to the previous
   failure in attempting to refresh the RRset.
6. Send a query for data.example txt.
7. Expect stale answer from cache due to stale-refresh-time
window being active, even if authoritative server is up.

Problem is that in step 4, due to the new option
stale-answer-client-timeout, recursive server will answer with stale
data before the actual fetch completes.

Since the original fetch is still running in background, if we re-enable
the authoritative server during that time, the RRset will actually be
successfully refreshed, and stale-refresh-window will not be activated.

The next queries will fail because they expect the TTL of the RRset to
match the one in the stale cache, not the one just refreshed.

To solve this, we explicitly disable stale-answer-client-timeout for
this test, as it's not the feature we are interested in testing here
anyways.
---
 bin/tests/system/serve-stale/clean.sh           | 1 +
 bin/tests/system/serve-stale/ns1/named1.conf.in | 1 +
 bin/tests/system/serve-stale/ns1/named2.conf.in | 1 +
 bin/tests/system/serve-stale/ns1/named3.conf.in | 1 +
 4 files changed, 4 insertions(+)

diff --git a/bin/tests/system/serve-stale/clean.sh b/bin/tests/system/serve-stale/clean.sh
index 5f3c90f540..2784ba25a0 100644
--- a/bin/tests/system/serve-stale/clean.sh
+++ b/bin/tests/system/serve-stale/clean.sh
@@ -15,3 +15,4 @@ rm -f */named.run */named.memstats
 rm -f ns*/managed-keys.bind*
 rm -f ns*/named_dump*
 rm -f ns*/named.stats*
+rm -f ns1/named.run.prev
diff --git a/bin/tests/system/serve-stale/ns1/named1.conf.in b/bin/tests/system/serve-stale/ns1/named1.conf.in
index 41347871cd..a8410a0e44 100644
--- a/bin/tests/system/serve-stale/ns1/named1.conf.in
+++ b/bin/tests/system/serve-stale/ns1/named1.conf.in
@@ -30,6 +30,7 @@ options {
 	max-stale-ttl 3600;
 	stale-answer-ttl 4;
 	stale-answer-enable yes;
+	stale-answer-client-timeout disabled;
 	stale-cache-enable yes;
 	stale-refresh-time 30;
 	servfail-ttl 0;
diff --git a/bin/tests/system/serve-stale/ns1/named2.conf.in b/bin/tests/system/serve-stale/ns1/named2.conf.in
index 06fae5369c..c8a8daeee1 100644
--- a/bin/tests/system/serve-stale/ns1/named2.conf.in
+++ b/bin/tests/system/serve-stale/ns1/named2.conf.in
@@ -30,6 +30,7 @@ options {
 	max-stale-ttl 20;
 	stale-answer-ttl 3;
 	stale-answer-enable yes;
+	stale-answer-client-timeout disabled;
 	stale-cache-enable yes;
 	servfail-ttl 0;
 };
diff --git a/bin/tests/system/serve-stale/ns1/named3.conf.in b/bin/tests/system/serve-stale/ns1/named3.conf.in
index f97dea958d..6fed86ae40 100644
--- a/bin/tests/system/serve-stale/ns1/named3.conf.in
+++ b/bin/tests/system/serve-stale/ns1/named3.conf.in
@@ -30,6 +30,7 @@ options {
 	max-stale-ttl 20;
 	stale-answer-ttl 3;
 	stale-answer-enable yes;
+	stale-answer-client-timeout disabled;
 	stale-cache-enable yes;
 	stale-refresh-time 0;
 	servfail-ttl 0;

From 0ad6f594f65c6f33879a6a942f8e8ed17ba14fef Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Fri, 18 Dec 2020 16:24:56 -0300
Subject: [PATCH 05/13] Added option for disabling stale-answer-client-timeout

This commit allows to specify "disabled" or "off" in
stale-answer-client-timeout statement. The logic to support this
behavior will be added in the subsequent commits.

This commit also ensures an upper bound to stale-answer-client-timeout
which equals to one second less than 'resolver-query-timeout'.
---
 bin/named/server.c     | 35 ++++++++++++++++++++++++++++++++++-
 lib/isccfg/namedconf.c | 25 ++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/bin/named/server.c b/bin/named/server.c
index 6888333087..19a0693fc5 100644
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -4488,7 +4488,19 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	obj = NULL;
 	result = named_config_get(maps, "stale-answer-client-timeout", &obj);
 	INSIST(result == ISC_R_SUCCESS);
-	view->staleanswerclienttimeout = cfg_obj_asuint32(obj);
+	if (cfg_obj_isstring(obj)) {
+		/*
+		 * The only string values available for this option
+		 * are "disabled" and "off".
+		 * We use (uint32_t) -1 to represent disabled since
+		 * a value of zero means that stale data can be used
+		 * to promptly answer the query, while an attempt to
+		 * refresh the RRset will still be made in background.
+		 */
+		view->staleanswerclienttimeout = (uint32_t)-1;
+	} else {
+		view->staleanswerclienttimeout = cfg_obj_asuint32(obj);
+	}
 
 	obj = NULL;
 	result = named_config_get(maps, "stale-refresh-time", &obj);
@@ -4779,6 +4791,27 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
 	query_timeout = cfg_obj_asuint32(obj);
 	dns_resolver_settimeout(view->resolver, query_timeout);
 
+	/*
+	 * Adjust stale-answer-client-timeout upper bound
+	 * to be resolver-query-timeout - 1s.
+	 * This assignment is safe as dns_resolver_settimeout()
+	 * ensures that resolver->querytimeout value will be in the
+	 * [MINIMUM_QUERY_TIMEOUT, MAXIMUM_QUERY_TIMEOUT] range and
+	 * MINIMUM_QUERY_TIMEOUT is > 1000 (in ms).
+	 */
+	if (view->staleanswerclienttimeout != (uint32_t)-1 &&
+	    view->staleanswerclienttimeout >
+		    (dns_resolver_gettimeout(view->resolver) - 1000))
+	{
+		view->staleanswerclienttimeout =
+			dns_resolver_gettimeout(view->resolver) - 1000;
+		isc_log_write(
+			named_g_lctx, NAMED_LOGCATEGORY_GENERAL,
+			NAMED_LOGMODULE_SERVER, ISC_LOG_WARNING,
+			"stale-answer-client-timeout adjusted to %" PRIu32,
+			view->staleanswerclienttimeout);
+	}
+
 	/* Specify whether to use 0-TTL for negative response for SOA query */
 	dns_resolver_setzeronosoattl(view->resolver, zero_no_soattl);
 
diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c
index 6b7c6fbabe..1ca22dd8d2 100644
--- a/lib/isccfg/namedconf.c
+++ b/lib/isccfg/namedconf.c
@@ -1899,6 +1899,28 @@ static cfg_type_t cfg_type_dns64 = { "dns64",	    cfg_parse_netprefix_map,
 				     cfg_print_map, cfg_doc_map,
 				     &cfg_rep_map,  dns64_clausesets };
 
+static const char *staleanswerclienttimeout_enums[] = { "disabled", "off",
+							NULL };
+static isc_result_t
+parse_staleanswerclienttimeout(cfg_parser_t *pctx, const cfg_type_t *type,
+			       cfg_obj_t **ret) {
+	return (cfg_parse_enum_or_other(pctx, type, &cfg_type_uint32, ret));
+}
+
+static void
+doc_staleanswerclienttimeout(cfg_printer_t *pctx, const cfg_type_t *type) {
+	cfg_doc_enum_or_other(pctx, type, &cfg_type_uint32);
+}
+
+static cfg_type_t cfg_type_staleanswerclienttimeout = {
+	"staleanswerclienttimeout",
+	parse_staleanswerclienttimeout,
+	cfg_print_ustring,
+	doc_staleanswerclienttimeout,
+	&cfg_rep_string,
+	staleanswerclienttimeout_enums
+};
+
 /*%
  * Clauses that can be found within the 'view' statement,
  * with defaults in the 'options' statement.
@@ -2027,7 +2049,8 @@ static cfg_clausedef_t view_clauses[] = {
 	{ "servfail-ttl", &cfg_type_duration, 0 },
 	{ "sortlist", &cfg_type_bracketed_aml, 0 },
 	{ "stale-answer-enable", &cfg_type_boolean, 0 },
-	{ "stale-answer-client-timeout", &cfg_type_uint32, 0 },
+	{ "stale-answer-client-timeout", &cfg_type_staleanswerclienttimeout,
+	  0 },
 	{ "stale-answer-ttl", &cfg_type_duration, 0 },
 	{ "stale-cache-enable", &cfg_type_boolean, 0 },
 	{ "stale-refresh-time", &cfg_type_duration, 0 },

From e219422575f25f431106cd5a4205a113e4b57f82 Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Mon, 21 Dec 2020 15:54:54 -0300
Subject: [PATCH 06/13] Allow stale data to be used before name resolution

This commit allows stale RRset to be used (if available) for responding
a query, before an attempt to refresh an expired, or otherwise resolve
an unavailable RRset in cache is made.

For that to work, a value of zero must be specified for
stale-answer-client-timeout statement.

To better understand the logic implemented, there are three flags being
used during database lookup and other parts of code that must be
understood:

. DNS_DBFIND_STALEOK: This flag is set when BIND fails to refresh a
  RRset due to timeout (resolver-query-timeout), its intent is to
  try to look for stale data in cache as a fallback, but only if
  stale answers are enabled in configuration.

  This flag is also used to activate stale-refresh-time window, since it
  is the only way the database knows that a resolution has failed.

. DNS_DBFIND_STALEENABLED: This flag is used as a hint to the database
  that it may use stale data. It is always set during query lookup if
  stale answers are enabled, but only effectively used during
  stale-refresh-time window. Also during this window, the resolver will
  not try to resolve the query, in other words no attempt to refresh the
  data in cache is made when the stale-refresh-time window is active.

. DNS_DBFIND_STALEONLY: This new introduced flag is used when we want
  stale data from the database, but not due to a failure in resolution,
  it also doesn't require stale-refresh-time window timer to be active.
  As long as there is a stale RRset available, it should be returned.
  It is mainly used in two situations:

    1. When stale-answer-client-timeout timer is triggered: in that case
       we want to know if there is stale data available to answer the
       client.
    2. When stale-answer-client-timeout value is set to zero: in that
       case, we also want to know if there is some stale RRset available
       to promptly answer the client.

We must also discern between three situations that may happen when
resolving a query after the addition of stale-answer-client-timeout
statement, and how to handle them:

	1. Are we running query_lookup() due to stale-answer-client-timeout
       timer being triggered?

       In this case, we look for stale data, making use of
       DNS_DBFIND_STALEONLY flag. If a stale RRset is available then
       respond the client with the data found, mark this query as
       answered (query attribute NS_QUERYATTR_ANSWERED), so when the
       fetch completes the client won't be answered twice.

       We must also take care of not detaching from the client, as a
       fetch will still be running in background, this is handled by the
       following snippet:

       if (!QUERY_STALEONLY(&client->query)) {
           isc_nmhandle_detach(&client->reqhandle);
       }

       Which basically tests if DNS_DBFIND_STALEONLY flag is set, which
       means we are here due to a stale-answer-client-timeout timer
       expiration.

    2. Are we running query_lookup() due to resolver-query-timeout being
       triggered?

       In this case, DNS_DBFIND_STALEOK flag will be set and an attempt
       to look for stale data will be made.
       As already explained, this flag is algo used to activate
       stale-refresh-time window, as it means that we failed to refresh
       a RRset due to timeout.
       It is ok in this situation to detach from the client, as the
       fetch is already completed.

    3. Are we running query_lookup() during the first time, looking for
       a RRset in cache and stale-answer-client-timeout value is set to
       zero?

       In this case, if stale answers are enabled (probably), we must do
       an initial database lookup with DNS_DBFIND_STALEONLY flag set, to
       indicate to the database that we want stale data.

       If we find an active RRset, proceed as normal, answer the client
       and the query is done.

       If we find a stale RRset we respond to the client and mark the
       query as answered, but don't detach from the client yet as an
       attempt in refreshing the RRset will still be made by means of
       the new introduced function 'query_resolve'.

       If no active or stale RRset is available, begin resolution as
       usual.
---
 lib/ns/client.c           |  11 +-
 lib/ns/include/ns/query.h |   1 +
 lib/ns/query.c            | 275 ++++++++++++++++++++++++++++++--------
 3 files changed, 229 insertions(+), 58 deletions(-)

diff --git a/lib/ns/client.c b/lib/ns/client.c
index 7cc96efc99..edd59197f2 100644
--- a/lib/ns/client.c
+++ b/lib/ns/client.c
@@ -421,6 +421,10 @@ ns_client_send(ns_client_t *client) {
 
 	REQUIRE(NS_CLIENT_VALID(client));
 
+	if ((client->query.attributes & NS_QUERYATTR_ANSWERED) != 0) {
+		return;
+	}
+
 	/*
 	 * XXXWPK TODO
 	 * Delay the response according to the -T delay option
@@ -670,6 +674,8 @@ renderend:
 				   ns_statscounter_truncatedresp);
 	}
 
+	client->query.attributes |= NS_QUERYATTR_ANSWERED;
+
 	return;
 
 cleanup:
@@ -879,9 +885,7 @@ ns_client_error(ns_client_t *client, isc_result_t result) {
 		}
 	}
 
-	if ((client->query.attributes & NS_QUERYATTR_ANSWERED) == 0) {
-		ns_client_send(client);
-	}
+	ns_client_send(client);
 }
 
 isc_result_t
@@ -2328,6 +2332,7 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) {
 					 .query = query };
 	}
 
+	client->query.attributes &= ~NS_QUERYATTR_ANSWERED;
 	client->state = NS_CLIENTSTATE_INACTIVE;
 	client->udpsize = 512;
 	client->ednsversion = -1;
diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h
index 55060a3a8c..74a95c9b94 100644
--- a/lib/ns/include/ns/query.h
+++ b/lib/ns/include/ns/query.h
@@ -118,6 +118,7 @@ struct ns_query {
 #define NS_QUERYATTR_RRL_CHECKED     0x10000
 #define NS_QUERYATTR_REDIRECT	     0x20000
 #define NS_QUERYATTR_ANSWERED	     0x40000
+#define NS_QUERYATTR_STALEOK	     0x80000
 
 typedef struct query_ctx query_ctx_t;
 
diff --git a/lib/ns/query.c b/lib/ns/query.c
index 9dcbc7ab7c..c3a8174ab4 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -86,11 +86,11 @@
  */
 #define MAX_RESTARTS 16
 
-#define QUERY_ERROR(qctx, r)                \
-	do {                                \
-		qctx->result = r;           \
-		qctx->want_restart = false; \
-		qctx->line = __LINE__;      \
+#define QUERY_ERROR(qctx, r)                  \
+	do {                                  \
+		(qctx)->result = r;           \
+		(qctx)->want_restart = false; \
+		(qctx)->line = __LINE__;      \
 	} while (0)
 
 /*% Partial answer? */
@@ -134,10 +134,10 @@
 #define REDIRECT(c) (((c)->query.attributes & NS_QUERYATTR_REDIRECT) != 0)
 
 /*% Was the query already answered due to stale-answer-client-timeout? */
-#define QUERY_ANSWERED(c) (((c)->query.attributes & NS_QUERYATTR_ANSWERED) != 0)
+#define QUERY_ANSWERED(q) (((q)->attributes & NS_QUERYATTR_ANSWERED) != 0)
 
 /*% Does the query only wants to check for stale RRset? */
-#define QUERY_STALEONLY(c) (((c)->query.dboptions & DNS_DBFIND_STALEONLY) != 0)
+#define QUERY_STALEONLY(q) (((q)->dboptions & DNS_DBFIND_STALEONLY) != 0)
 
 /*% Does the rdataset 'r' have an attached 'No QNAME Proof'? */
 #define NOQNAME(r) (((r)->attributes & DNS_RDATASETATTR_NOQNAME) != 0)
@@ -179,10 +179,11 @@ client_trace(ns_client_t *client, int level, const char *message) {
 #define CCTRACE(l, m) ((void)m)
 #endif /* WANT_QUERYTRACE */
 
-#define DNS_GETDB_NOEXACT   0x01U
-#define DNS_GETDB_NOLOG	    0x02U
-#define DNS_GETDB_PARTIAL   0x04U
-#define DNS_GETDB_IGNOREACL 0x08U
+#define DNS_GETDB_NOEXACT    0x01U
+#define DNS_GETDB_NOLOG	     0x02U
+#define DNS_GETDB_PARTIAL    0x04U
+#define DNS_GETDB_IGNOREACL  0x08U
+#define DNS_GETDB_STALEFIRST 0X0CU
 
 #define PENDINGOK(x) (((x)&DNS_DBFIND_PENDINGOK) != 0)
 
@@ -563,11 +564,9 @@ query_send(ns_client_t *client) {
 	}
 
 	inc_stats(client, counter);
-	if (!QUERY_ANSWERED(client)) {
-		ns_client_send(client);
-	}
+	ns_client_send(client);
 
-	if (!QUERY_STALEONLY(client)) {
+	if (!QUERY_STALEONLY(&client->query)) {
 		isc_nmhandle_detach(&client->reqhandle);
 	}
 }
@@ -597,7 +596,7 @@ query_error(ns_client_t *client, isc_result_t result, int line) {
 
 	ns_client_error(client, result);
 
-	if (!QUERY_STALEONLY(client)) {
+	if (!QUERY_STALEONLY(&client->query)) {
 		isc_nmhandle_detach(&client->reqhandle);
 	}
 }
@@ -613,7 +612,7 @@ query_next(ns_client_t *client, isc_result_t result) {
 	}
 	ns_client_drop(client, result);
 
-	if (!QUERY_STALEONLY(client)) {
+	if (!QUERY_STALEONLY(&client->query)) {
 		isc_nmhandle_detach(&client->reqhandle);
 	}
 }
@@ -5175,7 +5174,7 @@ qctx_freedata(query_ctx_t *qctx) {
 		dns_db_detach(&qctx->zdb);
 	}
 
-	if (qctx->event != NULL && !QUERY_STALEONLY(qctx->client)) {
+	if (qctx->event != NULL && !QUERY_STALEONLY(&qctx->client->query)) {
 		free_devent(qctx->client, ISC_EVENT_PTR(&qctx->event),
 			    &qctx->event);
 	}
@@ -5579,12 +5578,109 @@ ns__query_start(query_ctx_t *qctx) {
 		}
 	}
 
-	return (query_lookup(qctx));
+	if (!qctx->is_zone && (qctx->view->staleanswerclienttimeout == 0) &&
+	    dns_view_staleanswerenabled(qctx->view))
+	{
+		/*
+		 * If stale answers are enabled and
+		 * stale-answer-client-timeout is zero, then we can promptly
+		 * answer with a stale RRset if one is available in cache.
+		 */
+		qctx->options |= DNS_GETDB_STALEFIRST;
+	}
+
+	result = query_lookup(qctx);
+
+	/*
+	 * Clear "look-also-for-stale-data" flag.
+	 * If a fetch is created to resolve this query, then,
+	 * when it completes, this option is not expected to be set.
+	 */
+	qctx->options &= ~DNS_GETDB_STALEFIRST;
 
 cleanup:
 	return (result);
 }
 
+/*
+ * Setup a new query context for resolving a query.
+ *
+ * This function is only called if both these conditions are met:
+ *    1. BIND is configured with stale-answer-client-timeout 0.
+ *    2. A stale RRset is found in cache during initial query
+ *       database lookup.
+ *
+ * We continue with this function for refreshing/resolving an RRset
+ * after answering a client with stale data.
+ */
+static void
+query_refresh_rrset(query_ctx_t *qctx) {
+	isc_buffer_t b;
+	query_ctx_t new_qctx;
+
+	memset(&new_qctx, 0, sizeof(new_qctx));
+
+	dns_view_attach(qctx->view, &new_qctx.view);
+
+	new_qctx.qtype = new_qctx.type = qctx->qtype;
+	new_qctx.dns64 = qctx->dns64;
+	new_qctx.dns64_exclude = qctx->dns64_exclude;
+	new_qctx.client = qctx->client;
+	new_qctx.client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
+
+	/*
+	 * If it's a SIG query, we'll iterate the node.
+	 */
+	if (qctx->qtype == dns_rdatatype_rrsig ||
+	    qctx->qtype == dns_rdatatype_sig) {
+		new_qctx.type = dns_rdatatype_any;
+	}
+
+	new_qctx.dbuf = ns_client_getnamebuf(qctx->client);
+	if (ISC_UNLIKELY(new_qctx.dbuf == NULL)) {
+		CCTRACE(ISC_LOG_ERROR,
+			"query_refresh_rrset: ns_client_getnamebuf "
+			"failed");
+		goto done;
+	}
+
+	new_qctx.fname = ns_client_newname(new_qctx.client, new_qctx.dbuf, &b);
+	new_qctx.rdataset = ns_client_newrdataset(qctx->client);
+
+	if (ISC_UNLIKELY(new_qctx.fname == NULL || new_qctx.rdataset == NULL)) {
+		CCTRACE(ISC_LOG_ERROR,
+			"query_refresh_rrset: ns_client_newname failed");
+
+		goto done;
+	}
+
+	if ((WANTDNSSEC(qctx->client) || qctx->findcoveringnsec) &&
+	    (!qctx->is_zone || dns_db_issecure(qctx->db)))
+	{
+		new_qctx.sigrdataset = ns_client_newrdataset(qctx->client);
+		if (new_qctx.sigrdataset == NULL) {
+			CCTRACE(ISC_LOG_ERROR, "query_refresh_rrset: "
+					       "ns_client_newrdataset failed ");
+			goto done;
+		}
+	}
+
+	new_qctx.options = qctx->options;
+	new_qctx.result = ISC_R_SUCCESS;
+	new_qctx.findcoveringnsec = qctx->view->synthfromdnssec;
+
+	(void)query_gotanswer(&new_qctx, ISC_R_NOTFOUND);
+
+done:
+	if (new_qctx.fname != NULL) {
+		ns_client_releasename(new_qctx.client, &new_qctx.fname);
+	}
+	if (new_qctx.rdataset != NULL) {
+		ns_client_putrdataset(new_qctx.client, &new_qctx.rdataset);
+	}
+	qctx_destroy(&new_qctx);
+}
+
 /*%
  * Perform a local database lookup, in either an authoritative or
  * cache database. If unable to answer, call ns_query_done(); otherwise
@@ -5601,6 +5697,7 @@ query_lookup(query_ctx_t *qctx) {
 	dns_ttl_t stale_refresh = 0;
 	bool dbfind_stale = false;
 	bool stale_ok;
+	bool stale_used = false;
 
 	CCTRACE(ISC_LOG_DEBUG(3), "query_lookup");
 
@@ -5652,6 +5749,20 @@ query_lookup(query_ctx_t *qctx) {
 		rpzqname = qctx->client->query.qname;
 	}
 
+	if ((qctx->options & DNS_GETDB_STALEFIRST) != 0) {
+		/*
+		 * If DNS_GETDB_STALEFIRST is set, it means that stale
+		 * data may be returned as part of this lookup.
+		 * An attempt to refresh the RRset will still take
+		 * place if either an active RRset isn't available or
+		 * a stale one was found.
+		 * This is the expected behavior when
+		 * stale-answer-client-timeout value is zero and stale
+		 * answers are enabled.
+		 */
+		qctx->client->query.dboptions |= DNS_DBFIND_STALEONLY;
+	}
+
 	dboptions = qctx->client->query.dboptions;
 	if (!qctx->is_zone && qctx->findcoveringnsec &&
 	    (qctx->type != dns_rdatatype_null || !dns_name_istat(rpzqname)))
@@ -5702,21 +5813,19 @@ query_lookup(query_ctx_t *qctx) {
 	stale_ok = ((dboptions &
 		     (DNS_DBFIND_STALEENABLED | DNS_DBFIND_STALEONLY)) != 0);
 
-	if (dbfind_stale != 0 || (stale_ok && STALE(qctx->rdataset))) {
+	if (dbfind_stale || (stale_ok && STALE(qctx->rdataset))) {
 		char namebuf[DNS_NAME_FORMATSIZE];
-		bool success;
 
 		inc_stats(qctx->client, ns_statscounter_trystale);
 
-		qctx->client->query.dboptions &= ~DNS_DBFIND_STALEOK;
 		if (dns_rdataset_isassociated(qctx->rdataset) &&
 		    dns_rdataset_count(qctx->rdataset) > 0 &&
 		    STALE(qctx->rdataset))
 		{
 			qctx->rdataset->ttl = qctx->view->staleanswerttl;
-			success = true;
+			stale_used = true;
 		} else {
-			success = false;
+			stale_used = false;
 		}
 
 		dns_name_format(qctx->client->query.qname, namebuf,
@@ -5726,32 +5835,40 @@ query_lookup(query_ctx_t *qctx) {
 				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
 				      "%s resolver failure, stale answer %s",
 				      namebuf,
-				      success ? "used" : "unavailable");
+				      stale_used ? "used" : "unavailable");
+		} else if ((qctx->options & DNS_GETDB_STALEFIRST) != 0 &&
+			   stale_used) {
+			isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
+				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
+				      "%s stale answer used, an attempt to "
+				      "refresh the RRset will still be made",
+				      namebuf);
 		} else if ((dboptions & DNS_DBFIND_STALEONLY) != 0) {
 			isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
 				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
 				      "%s client timeout, stale answer %s",
 				      namebuf,
-				      success ? "used" : "unavailable");
+				      stale_used ? "used" : "unavailable");
 		} else {
 			isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
 				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
 				      "%s query within stale refresh time, "
 				      "stale answer %s",
 				      namebuf,
-				      success ? "used" : "unavailable");
+				      stale_used ? "used" : "unavailable");
 		}
 
-		if (!success) {
+		if (!stale_used &&
+		    ((qctx->options & DNS_GETDB_STALEFIRST) == 0)) {
 			/*
-			 * If DNS_DBFIND_STALEONLY is set then it means
-			 * stale-answer-client-timeout was triggered, in
-			 * that case we only want to check if a stale RRset is
-			 * available, if that's the case we promptly answer the
-			 * client with the stale data found. If a stale RRset is
-			 * not available then we must wait for the original
-			 * query to be resumed in order to build a proper
-			 * answer.
+			 * At this point, we know that stale data was not
+			 * available. A fetch may still be in progress to
+			 * add the data in cache, but if DNS_DBFIND_STALEONLY
+			 * is set, that means the client timeout was triggered.
+			 * But no answer was found, so we need to wait for the
+			 * original query to be resumed. If no client timeout
+			 * is active, then we have completed the fetch, or it
+			 * timed out, and we are done with the query.
 			 */
 			if ((dboptions & DNS_DBFIND_STALEONLY) == 0) {
 				QUERY_ERROR(qctx, DNS_R_SERVFAIL);
@@ -5760,18 +5877,54 @@ query_lookup(query_ctx_t *qctx) {
 
 			return (result);
 		}
+	} else {
+		/*
+		 * If we are here then either no stale RRset was found
+		 * or this is a regular query whose expected result is an
+		 * active RRset, ensure this flag won't affect further
+		 * database operations by disabling it.
+		 */
+		qctx->client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
 	}
 
 	/*
 	 * If DNS_DBFIND_STALEONLY is disabled then we proceed as normal,
 	 * otherwise we only proceed with query_gotanswer if we
-	 * successfully found a stale RRset in cache.
+	 * successfully found a stale RRset in cache, since this is an
+	 * attempt to find stale data after stale-answer-client-timeout
+	 * timer has expired. If no stale data was found then we must allow
+	 * a running fetch to complete in order to properly update the RRset.
+	 *
+	 * We must also ensure that if DNS_GETDB_STALEFIRST is set we won't
+	 * skip a call to query_gotanswer if we failed to find stale data,
+	 * since this means stale-answer-client-timeout is zero and we only
+	 * want to return stale data if any is available, otherwise we want
+	 * to resolve the query using the standard resolution process.
 	 */
-	if (((dboptions & DNS_DBFIND_STALEONLY) == 0) ||
-	    result == ISC_R_SUCCESS || result == DNS_R_GLUE ||
-	    result == DNS_R_ZONECUT)
+	if (result != ISC_R_SUCCESS &&
+	    ((dboptions & DNS_DBFIND_STALEONLY) != 0) &&
+	    ((qctx->options & DNS_GETDB_STALEFIRST) == 0))
 	{
-		return (query_gotanswer(qctx, result));
+		goto cleanup;
+	}
+
+	result = query_gotanswer(qctx, result);
+	stale_ok = (qctx->options & DNS_GETDB_STALEFIRST) != 0;
+
+	qctx->client->query.dboptions &= ~DNS_DBFIND_STALEOK;
+
+	if (stale_used && stale_ok) {
+		/*
+		 * If we reached this point then it means that we've
+		 * found a stale RRset entry in cache and BIND is
+		 * configured to allow queries to be answered with
+		 * stale data if no active RRset is available,
+		 * i.e. "stale-anwer-client-timeout 0".
+		 * We still need to refresh the RRset, a call to
+		 * query_refresh_rrset() is made to create a new query context
+		 * for that purpose.
+		 */
+		query_refresh_rrset(qctx);
 	}
 
 cleanup:
@@ -5786,24 +5939,19 @@ cleanup:
  * when the original fetch finishes.
  */
 static inline void
-query_lookup_staleonly(ns_client_t *client, dns_fetchevent_t *devent) {
+query_lookup_staleonly(ns_client_t *client) {
 	query_ctx_t qctx;
-	isc_result_t result;
 
-	qctx_init(client, &devent, client->query.qtype, &qctx);
+	qctx_init(client, NULL, client->query.qtype, &qctx);
 	dns_db_attach(client->view->cachedb, &qctx.db);
 	client->query.dboptions |= DNS_DBFIND_STALEONLY;
-	result = query_lookup(&qctx);
-	if (result == ISC_R_SUCCESS) {
-		client->query.attributes |= NS_QUERYATTR_ANSWERED;
-	}
+	(void)query_lookup(&qctx);
 	if (qctx.node != NULL) {
 		dns_db_detachnode(qctx.db, &qctx.node);
 	}
 	qctx_freedata(&qctx);
 	client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
 	qctx_destroy(&qctx);
-	isc_event_free(ISC_EVENT_PTR(&qctx.event));
 }
 
 /*
@@ -5836,8 +5984,11 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
 	CTRACE(ISC_LOG_DEBUG(3), "fetch_callback");
 
 	if (event->ev_type == DNS_EVENT_TRYSTALE) {
-		query_lookup_staleonly(client, devent);
+		query_lookup_staleonly(client);
+		isc_event_free(ISC_EVENT_PTR(&event));
 		return;
+	} else {
+		client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
 	}
 
 	LOCK(&client->query.fetchlock);
@@ -6160,7 +6311,10 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname,
 		peeraddr = &client->peeraddr;
 	}
 
-	if (dns_view_staleanswerenabled(client->view)) {
+	if (client->view->staleanswerclienttimeout > 0 &&
+	    client->view->staleanswerclienttimeout != (uint32_t)-1 &&
+	    dns_view_staleanswerenabled(client->view))
+	{
 		client->query.fetchoptions |= DNS_FETCHOPT_TRYSTALE_ONTIMEOUT;
 	}
 
@@ -7767,7 +7921,7 @@ query_addanswer(query_ctx_t *qctx) {
 		ns_client_putrdataset(qctx->client, &qctx->rdataset);
 	} else {
 		if (!qctx->is_zone && RECURSIONOK(qctx->client) &&
-		    !QUERY_STALEONLY(qctx->client))
+		    !QUERY_STALEONLY(&qctx->client->query))
 		{
 			query_prefetch(qctx->client, qctx->fname,
 				       qctx->rdataset);
@@ -7876,7 +8030,7 @@ query_respond(query_ctx_t *qctx) {
 	 * We shouldn't ever fail to add 'rdataset'
 	 * because it's already in the answer.
 	 */
-	INSIST(qctx->rdataset == NULL || QUERY_ANSWERED(qctx->client));
+	INSIST(qctx->rdataset == NULL || QUERY_ANSWERED(&qctx->client->query));
 
 	query_addauth(qctx);
 
@@ -11293,6 +11447,7 @@ isc_result_t
 ns_query_done(query_ctx_t *qctx) {
 	isc_result_t result = ISC_R_UNSET;
 	const dns_namelist_t *secs = qctx->client->message->sections;
+	bool query_stale_only;
 
 	CCTRACE(ISC_LOG_DEBUG(3), "ns_query_done");
 
@@ -11361,7 +11516,10 @@ ns_query_done(query_ctx_t *qctx) {
 	 * If we're recursing then just return; the query will
 	 * resume when recursion ends.
 	 */
-	if (RECURSING(qctx->client) && !QUERY_STALEONLY(qctx->client)) {
+	if (RECURSING(qctx->client) &&
+	    (!QUERY_STALEONLY(&qctx->client->query) ||
+	     ((qctx->options & DNS_GETDB_STALEFIRST) != 0)))
+	{
 		return (qctx->result);
 	}
 
@@ -11397,9 +11555,16 @@ ns_query_done(query_ctx_t *qctx) {
 
 	CALL_HOOK(NS_QUERY_DONE_SEND, qctx);
 
+	/*
+	 * Client may have been detached after query_send(), so
+	 * we test and store the flag state here, for safety.
+	 */
+	query_stale_only = QUERY_STALEONLY(&qctx->client->query);
 	query_send(qctx->client);
 
-	qctx->detach_client = true;
+	if (!query_stale_only) {
+		qctx->detach_client = true;
+	}
 	return (qctx->result);
 
 cleanup:

From 35fd039d038419d0301737f3a9afdd2310a81234 Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Wed, 23 Dec 2020 10:47:02 -0300
Subject: [PATCH 07/13] Add system tests for stale-answer-client-timeout

This commit add 4 tests for the new option:
	1. Test default configuration of stale-answer-client-timeout, a
	   value of 1.8 seconds, with stale-refresh-time disabled.

	2. Test disabling of stale-answer-client-timeout.

	3. Test stale-answer-client-timeout with a value of zero, in this
	   case we take advantage of a log entry which shows that a stale
	   answer was promptly used before an attempt to refresh the RRset
	   is made. We also check, by activating a disabled authoritative
	   server, that the RRset was successfully refreshed after that.

	4. Test stale-answer-client-timeout 0 with stale-refresh-time 4, in
	   this test we want to ensure a couple things:

	   - If we have a stale RRSet entry in cache, a request must be
		 promptly answered with this data, while BIND must also attempt
		 to refresh the RRSet in background.

	   - If the attempt to refresh the RRSet times out, the RRSet must
		 have its stale-refresh-time window activated.

	   - If a new request for the same RRSet arrives, it must be
		 promptly answered with stale data due to stale-refresh-time
		 being active for this RRSet, in this case no attempt to refresh
		 the RRSet is made.

	   - Enable authoritative server, ensure that the RRSet was not
		 refreshed, to honor stale-refresh-time.

	   - Wait for stale-refresh-window time pass, send another request
		 for the same RRSet, this time we expect the answer to be the
		 stale entry in cache being hit due to
		 stale-answer-client-timeout 0.

	    - Send another request, this time we expect the answer to be an
		  active RRSet, since it must have been refreshed during the
		  previous request.
---
 bin/tests/system/serve-stale/clean.sh         |   2 +-
 .../system/serve-stale/ns3/named2.conf.in     |  48 ++
 .../system/serve-stale/ns3/named3.conf.in     |  48 ++
 .../system/serve-stale/ns3/named4.conf.in     |  48 ++
 .../system/serve-stale/ns3/named5.conf.in     |  48 ++
 bin/tests/system/serve-stale/ns3/root.db      |  11 +
 bin/tests/system/serve-stale/tests.sh         | 466 +++++++++++++++++-
 7 files changed, 667 insertions(+), 4 deletions(-)
 create mode 100644 bin/tests/system/serve-stale/ns3/named2.conf.in
 create mode 100644 bin/tests/system/serve-stale/ns3/named3.conf.in
 create mode 100644 bin/tests/system/serve-stale/ns3/named4.conf.in
 create mode 100644 bin/tests/system/serve-stale/ns3/named5.conf.in
 create mode 100644 bin/tests/system/serve-stale/ns3/root.db

diff --git a/bin/tests/system/serve-stale/clean.sh b/bin/tests/system/serve-stale/clean.sh
index 2784ba25a0..a089574d37 100644
--- a/bin/tests/system/serve-stale/clean.sh
+++ b/bin/tests/system/serve-stale/clean.sh
@@ -15,4 +15,4 @@ rm -f */named.run */named.memstats
 rm -f ns*/managed-keys.bind*
 rm -f ns*/named_dump*
 rm -f ns*/named.stats*
-rm -f ns1/named.run.prev
+rm -f ns*/named.run.prev
diff --git a/bin/tests/system/serve-stale/ns3/named2.conf.in b/bin/tests/system/serve-stale/ns3/named2.conf.in
new file mode 100644
index 0000000000..e91f67b044
--- /dev/null
+++ b/bin/tests/system/serve-stale/ns3/named2.conf.in
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+
+/*
+ * Test default stale-answer-client-timeout value
+ */
+
+key rndc_key {
+	secret "1234abcd8765";
+	algorithm hmac-sha256;
+};
+
+controls {
+	inet 10.53.0.3 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
+};
+
+options {
+	query-source address 10.53.0.3;
+	notify-source 10.53.0.3;
+	transfer-source 10.53.0.3;
+	port @PORT@;
+	pid-file "named.pid";
+	listen-on { 10.53.0.3; };
+	listen-on-v6 { none; };
+	dnssec-validation no;
+	recursion yes;
+	stale-answer-enable yes;
+	stale-cache-enable yes;
+	stale-answer-ttl 3;
+	stale-refresh-time 0;
+	max-stale-ttl 3600;
+	resolver-query-timeout 10;
+};
+
+zone "." {
+	type secondary;
+	primaries { 10.53.0.1; };
+	file "root.bk";
+};
diff --git a/bin/tests/system/serve-stale/ns3/named3.conf.in b/bin/tests/system/serve-stale/ns3/named3.conf.in
new file mode 100644
index 0000000000..0520e514c1
--- /dev/null
+++ b/bin/tests/system/serve-stale/ns3/named3.conf.in
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+
+/*
+ * Test disable of stale-answer-client-timeout.
+ */
+
+key rndc_key {
+	secret "1234abcd8765";
+	algorithm hmac-sha256;
+};
+
+controls {
+	inet 10.53.0.3 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
+};
+
+options {
+	query-source address 10.53.0.3;
+	notify-source 10.53.0.3;
+	transfer-source 10.53.0.3;
+	port @PORT@;
+	pid-file "named.pid";
+	listen-on { 10.53.0.3; };
+	listen-on-v6 { none; };
+	dnssec-validation no;
+	recursion yes;
+	stale-answer-enable yes;
+	stale-cache-enable yes;
+	stale-answer-ttl 3;
+	stale-answer-client-timeout off;
+	stale-refresh-time 0;
+	max-stale-ttl 3600;
+	resolver-query-timeout 10;
+};
+
+zone "." {
+	type hint;
+	file "root.db";
+};
diff --git a/bin/tests/system/serve-stale/ns3/named4.conf.in b/bin/tests/system/serve-stale/ns3/named4.conf.in
new file mode 100644
index 0000000000..5fd5ef82f4
--- /dev/null
+++ b/bin/tests/system/serve-stale/ns3/named4.conf.in
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+
+/*
+ * Test stale-answer-client-timeout 0.
+ */
+
+key rndc_key {
+	secret "1234abcd8765";
+	algorithm hmac-sha256;
+};
+
+controls {
+	inet 10.53.0.3 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
+};
+
+options {
+	query-source address 10.53.0.3;
+	notify-source 10.53.0.3;
+	transfer-source 10.53.0.3;
+	port @PORT@;
+	pid-file "named.pid";
+	listen-on { 10.53.0.3; };
+	listen-on-v6 { none; };
+	dnssec-validation no;
+	recursion yes;
+	stale-answer-enable yes;
+	stale-cache-enable yes;
+	stale-answer-ttl 3;
+	stale-answer-client-timeout 0;
+	stale-refresh-time 0;
+	resolver-query-timeout 10;
+	max-stale-ttl 3600;
+};
+
+zone "." {
+	type hint;
+	file "root.db";
+};
diff --git a/bin/tests/system/serve-stale/ns3/named5.conf.in b/bin/tests/system/serve-stale/ns3/named5.conf.in
new file mode 100644
index 0000000000..92b2cdf108
--- /dev/null
+++ b/bin/tests/system/serve-stale/ns3/named5.conf.in
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+
+/*
+ * Test stale-answer-client-timeout 0.
+ */
+
+key rndc_key {
+	secret "1234abcd8765";
+	algorithm hmac-sha256;
+};
+
+controls {
+	inet 10.53.0.3 port @CONTROLPORT@ allow { any; } keys { rndc_key; };
+};
+
+options {
+	query-source address 10.53.0.3;
+	notify-source 10.53.0.3;
+	transfer-source 10.53.0.3;
+	port @PORT@;
+	pid-file "named.pid";
+	listen-on { 10.53.0.3; };
+	listen-on-v6 { none; };
+	dnssec-validation no;
+	recursion yes;
+	stale-answer-enable yes;
+	stale-cache-enable yes;
+	stale-answer-ttl 3;
+	stale-answer-client-timeout 0;
+	stale-refresh-time 4;
+	resolver-query-timeout 10;
+	max-stale-ttl 3600;
+};
+
+zone "." {
+	type hint;
+	file "root.db";
+};
diff --git a/bin/tests/system/serve-stale/ns3/root.db b/bin/tests/system/serve-stale/ns3/root.db
new file mode 100644
index 0000000000..8f779b0878
--- /dev/null
+++ b/bin/tests/system/serve-stale/ns3/root.db
@@ -0,0 +1,11 @@
+; Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+;
+; This Source Code Form is subject to the terms of the Mozilla Public
+; License, v. 2.0. If a copy of the MPL was not distributed with this
+; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+;
+; See the COPYRIGHT file distributed with this work for additional
+; information regarding copyright ownership.
+
+.           300 NS  ns.nil.
+ns.nil.     300 A   10.53.0.1
diff --git a/bin/tests/system/serve-stale/tests.sh b/bin/tests/system/serve-stale/tests.sh
index 5ede9c51e1..8e009c3839 100755
--- a/bin/tests/system/serve-stale/tests.sh
+++ b/bin/tests/system/serve-stale/tests.sh
@@ -19,7 +19,6 @@ stale_answer_ttl=$(sed -ne 's,^[[:space:]]*stale-answer-ttl \([[:digit:]]*\).*,\
 
 status=0
 n=0
-
 #
 # First test server with serve-stale options set.
 #
@@ -188,7 +187,7 @@ if [ $ret != 0 ]; then echo_i "failed"; fi
 # 2. Disable responses from authoritative server.
 # 3. Sleep for TTL duration so rrset TTL expires (2 sec)
 # 4. Query data.example
-# 5. Check if response come from stale rrset (3 sec TTL)
+# 5. Check if response come from stale rrset (4 sec TTL)
 # 6. Enable responses from authoritative server.
 # 7. Query data.example
 # 8. Check if response come from stale rrset, since the query
@@ -885,7 +884,7 @@ $DIG -p ${PORT} @10.53.0.1 data.example TXT > dig.out.test$((n+1))
 
 # Step 8.
 n=$((n+1))
-echo_i "check stale data.example comes from authoritative (stale-refresh-time disabled) ($n)"
+echo_i "check data.example comes from authoritative (stale-refresh-time disabled) ($n)"
 ret=0
 grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
 grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
@@ -1553,5 +1552,466 @@ grep -F "#NXDOMAIN" ns5/named.stats.$n.cachedb > /dev/null && ret=1
 status=$((status+ret))
 if [ $ret != 0 ]; then echo_i "failed"; fi
 
+########################################################
+# Test for stale-answer-client-timeout (default 1.8s). #
+########################################################
+echo_i "test stale-answer-client-timeout (default 1.8)"
+
+n=$((n+1))
+echo_i "updating ns3/named.conf ($n)"
+ret=0
+copy_setports ns3/named2.conf.in ns3/named.conf
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+echo_i "restart ns3"
+$PERL ../stop.pl --use-rndc --port ${CONTROLPORT} serve-stale ns3
+start_server --noclean --restart --port ${PORT} serve-stale ns3
+
+n=$((n+1))
+echo_i "check 'rndc serve-stale status' ($n)"
+ret=0
+$RNDCCMD 10.53.0.3 serve-stale status > rndc.out.test$n 2>&1 || ret=1
+grep '_default: on (stale-answer-ttl=3 max-stale-ttl=3600 stale-refresh-time=0)' rndc.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "enable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt enable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"1\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "prime cache data.example (stale-answer-client-timeout)"
+ret=0
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "prime cache nodata.example (stale-answer-client-timeout)"
+ret=0
+$DIG -p ${PORT} @10.53.0.3 nodata.example TXT > dig.out.test$n
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 0," dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "disable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt disable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"0\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# Allow RRset to become stale.
+sleep 2
+
+# We configured a long value of 30 seconds for resolver-query-timeout.
+# That should give us enough time to receive an stale answer from cache
+# after stale-answer-client-timeout timer of 1.8 sec triggers.
+n=$((n+1))
+echo_i "check stale data.example comes from cache (default stale-answer-client-timeout) ($n)"
+nextpart ns3/named.run > /dev/null
+t1=`$PERL -e 'print time()'`
+$DIG -p ${PORT} +tries=1 +timeout=10  @10.53.0.3 data.example TXT > dig.out.test$n
+t2=`$PERL -e 'print time()'`
+wait_for_log 5 "data.example client timeout, stale answer used" ns3/named.run || ret=1
+ret=0
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*3.*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+# Default stale-answer-client-timeout is 1.8s, we allow some extra time
+# just in case other tests are taking too much cpu.
+[ $((t2 - t1)) -le 10 ] || { echo_i "query took $((t2 - t1))s to resolve.";  ret=1; }
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+echo_i "sending queries for tests $((n+1))-$((n+2))..."
+$DIG -p ${PORT} +tries=1 +timeout=3  @10.53.0.3 nodata.example TXT > dig.out.test$((n+1)) &
+$DIG -p ${PORT} +tries=1 +timeout=30  @10.53.0.3 nodata.example TXT > dig.out.test$((n+2))
+wait
+
+# Since nodata.example is cached as NXRRSET and marked as
+# stale at this point, BIND must not return this RRset when
+# stale-answer-client-timeout triggers, instead, it must attempt
+# to refresh the RRset.
+# Since the authoritative server is disabled and we are using
+# resolver-query-timeout value of 10 seconds, we expect this
+# query with a timeout of 3 seconds to time out.
+n=$((n+1))
+echo_i "check query for nodata.example times out (default stale-answer-client-timeout) ($n)"
+grep "connection timed out" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# For this query we expect BIND to return stale NXRRSET data
+# for nodata.example after resolver-query-timeout expires.
+n=$((n+1))
+echo_i "check stale nodata.example comes from cache after resolver-query-timeout expires (default stale-answer-client-timeout) ($n)"
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 0," dig.out.test$n > /dev/null || ret=1
+grep "example\..*3.*IN.*SOA" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+#############################################
+# Test for stale-answer-client-timeout off. #
+#############################################
+echo_i "test stale-answer-client-timeout (off)"
+
+n=$((n+1))
+echo_i "updating ns3/named.conf ($n)"
+ret=0
+copy_setports ns3/named3.conf.in ns3/named.conf
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "running 'rndc reload' ($n)"
+ret=0
+rndc_reload ns3 10.53.0.3
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# Send a query, auth server is disabled, we will enable it after
+# a while in order to receive an answer before resolver-query-timeout
+# expires. Since stale-answer-client-timeout is disabled we must receive
+# an answer from authoritative server.
+echo_i "sending query for test $((n+2))"
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$((n+2)) &
+sleep 3
+
+n=$((n+1))
+echo_i "enable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt enable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"1\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# Wait until dig is done.
+wait
+
+n=$((n+1))
+echo_i "check data.example comes from authoritative server (stale-answer-client-timeout off) ($n)"
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*[12].*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+#############################################
+# Test for stale-answer-client-timeout 0.   #
+#############################################
+echo_i "test stale-answer-client-timeout (0)"
+
+n=$((n+1))
+echo_i "updating ns3/named.conf ($n)"
+ret=0
+copy_setports ns3/named4.conf.in ns3/named.conf
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+echo_i "restart ns3"
+$PERL ../stop.pl --use-rndc --port ${CONTROLPORT} serve-stale ns3
+start_server --noclean --restart --port ${PORT} serve-stale ns3
+
+n=$((n+1))
+echo_i "prime cache data.example (stale-answer-client-timeout 0)"
+ret=0
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "prime cache nodata.example (stale-answer-client-timeout 0)"
+ret=0
+$DIG -p ${PORT} @10.53.0.3 nodata.example TXT > dig.out.test$n
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 0," dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "disable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt disable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"0\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# Allow RRset to become stale.
+sleep 2
+
+n=$((n+1))
+ret=0
+echo_i "check stale data.example comes from cache (stale-answer-client-timeout 0) ($n)"
+nextpart ns3/named.run > /dev/null
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+wait_for_log 5 "data.example stale answer used, an attempt to refresh the RRset" ns3/named.run || ret=1
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*3.*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "enable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt enable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"1\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+wait_for_rrset_refresh() {
+	nextpart ns3/named.run | grep 'data.example.*2.*TXT.*"A text record with a 2 second ttl"' > /dev/null && return 0
+	return 1
+}
+
+# This test ensures that after we get stale data due to
+# stale-answer-client-timeout 0, enabling the authoritative server
+# will allow the RRset to be updated.
+n=$((n+1))
+ret=0
+echo_i "check stale data.example was refreshed (stale-answer-client-timeout 0) ($n)"
+retry_quiet 10 wait_for_rrset_refresh || ret=1
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*[12].*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "disable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt disable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"0\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+echo_i "sending queries for tests $((n+1))-$((n+2))..."
+$DIG -p ${PORT} +tries=1 +timeout=3  @10.53.0.3 nodata.example TXT > dig.out.test$((n+1)) &
+$DIG -p ${PORT} +tries=1 +timeout=30  @10.53.0.3 nodata.example TXT > dig.out.test$((n+2))
+wait
+
+# Since nodata.example is cached as NXRRSET and marked as
+# stale at this point, BIND must not prompty return this RRset
+# due to stale-answer-client-timeout == 0, instead, it must
+# attempt to refresh the RRset.
+# Since the authoritative server is disabled and we are using
+# resolver-query-timeout value of 10 seconds, we expect this
+# query with a timeout of 3 seconds to time out.
+n=$((n+1))
+echo_i "check query for nodata.example times out (stale-answer-client-timeout 0) ($n)"
+grep "connection timed out" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# For this query we expect BIND to return stale NXRRSET data
+# for nodata.example after resolver-query-timeout expires.
+n=$((n+1))
+echo_i "check stale nodata.example comes from cache after resolver-query-timeout expires (stale-answer-client-timeout 0) ($n)"
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 0," dig.out.test$n > /dev/null || ret=1
+grep "example\..*3.*IN.*SOA" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+####################################################################
+# Test for stale-answer-client-timeout 0 and stale-refresh-time 4. #
+####################################################################
+echo_i "test stale-answer-client-timeout (0) and stale-refresh-time (4)"
+
+n=$((n+1))
+echo_i "updating ns3/named.conf ($n)"
+ret=0
+copy_setports ns3/named5.conf.in ns3/named.conf
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "running 'rndc reload' ($n)"
+ret=0
+rndc_reload ns3 10.53.0.3
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "flush cache, enable responses from authoritative server ($n)"
+ret=0
+$RNDCCMD 10.53.0.3 flushtree example > rndc.out.test$n.1 2>&1 || ret=1
+$DIG -p ${PORT} @10.53.0.2 txt enable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"1\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "prime cache data.example (stale-answer-client-timeout 0, stale-refresh-time 4) ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*2.*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# Allow RRset to become stale.
+sleep 2
+
+n=$((n+1))
+echo_i "disable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt disable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"0\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+ret=0
+echo_i "check stale data.example comes from cache (stale-answer-client-timeout 0 stale-refresh-time 4) ($n)"
+nextpart ns3/named.run > /dev/null
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+wait_for_log 5 "data.example stale answer used, an attempt to refresh the RRset" ns3/named.run || ret=1
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*3.*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "enable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt enable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"1\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# This test ensures that after we get stale data due to
+# stale-answer-client-timeout 0, enabling the authoritative server
+# will allow the RRset to be updated.
+n=$((n+1))
+ret=0
+echo_i "check stale data.example was refreshed (stale-answer-client-timeout 0 stale-refresh-time 4) ($n)"
+retry_quiet 10 wait_for_rrset_refresh || ret=1
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*[12].*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# Allow RRset to become stale.
+sleep 2
+
+n=$((n+1))
+echo_i "disable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt disable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"0\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+ret=0
+echo_i "check stale data.example comes from cache (stale-answer-client-timeout 0 stale-refresh-time 4) ($n)"
+nextpart ns3/named.run > /dev/null
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+wait_for_log 5 "data.example stale answer used, an attempt to refresh the RRset" ns3/named.run || ret=1
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*3.*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# Allow stale-refresh-time to be activated.
+n=$((n+1))
+ret=0
+echo_i "wait until resolver query times out, activating stale-refresh-time"
+wait_for_log 15 "data.example resolver failure, stale answer used" ns3/named.run || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+ret=0
+echo_i "check stale data.example comes from cache within stale-refresh-time (stale-answer-client-timeout 0 stale-refresh-time 4) ($n)"
+nextpart ns3/named.run > /dev/null
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+wait_for_log 5 "data.example query within stale refresh time" ns3/named.run || ret=1
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*3.*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+echo_i "enable responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt enable  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"1\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# We give BIND some time to ensure that after we enable authoritative server,
+# this RRset is still not refreshed because it was hit during
+# stale-refresh-time window.
+sleep 1
+
+n=$((n+1))
+ret=0
+echo_i "check stale data.example was not refreshed (stale-answer-client-timeout 0 stale-refresh-time 4) ($n)"
+nextpart ns3/named.run > /dev/null
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+wait_for_log 5 "data.example query within stale refresh time" ns3/named.run || ret=1
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*3.*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# After the refresh-time-window, the RRset will be refreshed.
+sleep 4
+
+n=$((n+1))
+ret=0
+echo_i "check stale data.example comes from cache (stale-answer-client-timeout 0 stale-refresh-time 4) ($n)"
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+wait_for_log 5 "data.example stale answer used, an attempt to refresh the RRset" ns3/named.run || ret=1
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*3.*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+n=$((n+1))
+ret=0
+echo_i "check stale data.example was refreshed (stale-answer-client-timeout 0 stale-refresh-time 4) ($n)"
+$DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$n
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "data\.example\..*[12].*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
 echo_i "exit status: $status"
 [ $status -eq 0 ] || exit 1

From 6ab907045762b3a9ffd8595808197c4e05b7d3cc Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Wed, 23 Dec 2020 12:16:26 -0300
Subject: [PATCH 08/13] Add documentation for stale-answer-client-timeout

---
 bin/named/named.conf.rst     |  2 ++
 doc/arm/reference.rst        | 14 ++++++++++++++
 doc/man/named.conf.5in       |  2 ++
 doc/misc/options             |  2 ++
 doc/misc/options.active      |  2 ++
 doc/misc/options.grammar.rst |  1 +
 doc/notes/notes-current.rst  | 17 +++++++++++++++++
 7 files changed, 40 insertions(+)

diff --git a/bin/named/named.conf.rst b/bin/named/named.conf.rst
index 9fbe08b0ba..40163878a5 100644
--- a/bin/named/named.conf.rst
+++ b/bin/named/named.conf.rst
@@ -403,6 +403,7 @@ OPTIONS
   	sig-validity-interval integer [ integer ];
   	sortlist { address_match_element; ... };
   	stacksize ( default | unlimited | sizeval );
+  	stale-answer-client-timeout ( disabled | off | integer );
   	stale-answer-enable boolean;
   	stale-answer-ttl duration;
   	stale-cache-enable boolean;
@@ -806,6 +807,7 @@ VIEW
   	sig-signing-type integer;
   	sig-validity-interval integer [ integer ];
   	sortlist { address_match_element; ... };
+  	stale-answer-client-timeout ( disabled | off | integer );
   	stale-answer-enable boolean;
   	stale-answer-ttl duration;
   	stale-cache-enable boolean;
diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst
index 96416d320e..eb9140b3dc 100644
--- a/doc/arm/reference.rst
+++ b/doc/arm/reference.rst
@@ -1832,6 +1832,20 @@ Boolean Options
    Information about stale answers is logged under the ``serve-stale``
    log category.
 
+``stale-answer-client-timeout``
+   This option defines the amount of time ``named`` waits before attempting to
+   answer the query with a stale RRset from cache. If a stale answer is found,
+   ``named`` continues the ongoing fetches, attempting to refresh the RRset in
+   cache until the ``resolver-query-timeout`` interval is reached.
+
+   The default value is ``1800`` (in milliseconds) and the maximum value is
+   bounded to ``resolver-query-timeout`` minus one second. A value of ``0``
+   immediately returns a cached RRset if available, and still attempts a refresh
+   of the data in cache.
+
+   The option can be disabled by setting the value to ``off`` or ``disabled``.
+   It also has no effect if ``stale-answer-enable`` is disabled.
+
 ``stale-cache-enable``
    If ``yes``, enable the retaining of "stale" cached answers.  Default ``no``.
 
diff --git a/doc/man/named.conf.5in b/doc/man/named.conf.5in
index 9798295f4a..2180bdbd6c 100644
--- a/doc/man/named.conf.5in
+++ b/doc/man/named.conf.5in
@@ -466,6 +466,7 @@ options {
       sig\-validity\-interval integer [ integer ];
       sortlist { address_match_element; ... };
       stacksize ( default | unlimited | sizeval );
+      stale\-answer\-client\-timeout ( disabled | off | integer );
       stale\-answer\-enable boolean;
       stale\-answer\-ttl duration;
       stale\-cache\-enable boolean;
@@ -901,6 +902,7 @@ view string [ class ] {
       sig\-signing\-type integer;
       sig\-validity\-interval integer [ integer ];
       sortlist { address_match_element; ... };
+      stale\-answer\-client\-timeout ( disabled | off | integer );
       stale\-answer\-enable boolean;
       stale\-answer\-ttl duration;
       stale\-cache\-enable boolean;
diff --git a/doc/misc/options b/doc/misc/options
index 33874c2d0c..3444f6eaf8 100644
--- a/doc/misc/options
+++ b/doc/misc/options
@@ -333,6 +333,7 @@ options {
         sig-validity-interval <integer> [ <integer> ];
         sortlist { <address_match_element>; ... };
         stacksize ( default | unlimited | <sizeval> );
+        stale-answer-client-timeout ( disabled | off | <integer> );
         stale-answer-enable <boolean>;
         stale-answer-ttl <duration>;
         stale-cache-enable <boolean>;
@@ -696,6 +697,7 @@ view <string> [ <class> ] {
         sig-signing-type <integer>;
         sig-validity-interval <integer> [ <integer> ];
         sortlist { <address_match_element>; ... };
+        stale-answer-client-timeout ( disabled | off | <integer> );
         stale-answer-enable <boolean>;
         stale-answer-ttl <duration>;
         stale-cache-enable <boolean>;
diff --git a/doc/misc/options.active b/doc/misc/options.active
index 6d158267bf..deeda67131 100644
--- a/doc/misc/options.active
+++ b/doc/misc/options.active
@@ -331,6 +331,7 @@ options {
         sig-validity-interval <integer> [ <integer> ];
         sortlist { <address_match_element>; ... };
         stacksize ( default | unlimited | <sizeval> );
+        stale-answer-client-timeout ( disabled | off | <integer> );
         stale-answer-enable <boolean>;
         stale-answer-ttl <duration>;
         stale-cache-enable <boolean>;
@@ -692,6 +693,7 @@ view <string> [ <class> ] {
         sig-signing-type <integer>;
         sig-validity-interval <integer> [ <integer> ];
         sortlist { <address_match_element>; ... };
+        stale-answer-client-timeout ( disabled | off | <integer> );
         stale-answer-enable <boolean>;
         stale-answer-ttl <duration>;
         stale-cache-enable <boolean>;
diff --git a/doc/misc/options.grammar.rst b/doc/misc/options.grammar.rst
index 867256d25a..4fdc098aae 100644
--- a/doc/misc/options.grammar.rst
+++ b/doc/misc/options.grammar.rst
@@ -259,6 +259,7 @@
   	sig-validity-interval <integer> [ <integer> ];
   	sortlist { <address_match_element>; ... };
   	stacksize ( default | unlimited | <sizeval> );
+  	stale-answer-client-timeout ( disabled | off | <integer> );
   	stale-answer-enable <boolean>;
   	stale-answer-ttl <duration>;
   	stale-cache-enable <boolean>;
diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst
index 15067ac56d..9044417d92 100644
--- a/doc/notes/notes-current.rst
+++ b/doc/notes/notes-current.rst
@@ -26,6 +26,23 @@ New Features
 
 - None.
 
+- A new option, ```stale-answer-client-timeout``, has been added to
+  improve ``named``'s behavior with respect to serving stale data. The option
+  defines the amount of time ``named`` waits before attempting
+  to answer the query with a stale RRset from cache. If a stale answer
+  is found, ``named`` continues the ongoing fetches, attempting to
+  refresh the RRset in cache until the ``resolver-query-timeout`` interval is
+  reached.
+
+  The default value is ``1800`` (in milliseconds) and the maximum value is
+  bounded to ``resolver-query-timeout`` minus one second. A value of
+  ``0`` immediately returns a cached RRset if available, and still
+  attempts a refresh of the data in cache.
+
+  The option can be disabled by setting the value to ``off`` or
+  ``disabled``. It also has no effect if ``stale-answer-enable`` is
+  disabled.
+
 Removed Features
 ~~~~~~~~~~~~~~~~
 

From 42c789c763689f20cb0704977254bc105e9a7a1d Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Wed, 23 Dec 2020 12:25:41 -0300
Subject: [PATCH 09/13] Add CHANGES note for [GL #2247]

---
 CHANGES | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/CHANGES b/CHANGES
index cf0b4945be..987862f2d8 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,8 @@
+5566.	[func]		Add "stale-answer-client-timeout" option, which
+			is the amount of time a recursive resolver waits before
+			attempting to answer the query using stale data from cache.
+			[GL #2247]
+
 5565.	[func]		The SONAMEs for BIND 9 libraries now include the current
 			BIND 9 version number, in an effort to tightly couple
 			internal libraries with a specific release. [GL #2387]

From f89ac07b28847d65a2b81b5474504b4634aeaf35 Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Tue, 12 Jan 2021 12:59:21 -0300
Subject: [PATCH 10/13] Small optimization in query_usestale

This commit makes the code in query_usestale easier to follow, it also
doesn't attach/detach to the database if stale answers are not enabled.
---
 lib/ns/query.c | 35 +++++------------------------------
 1 file changed, 5 insertions(+), 30 deletions(-)

diff --git a/lib/ns/query.c b/lib/ns/query.c
index c3a8174ab4..b452068f1a 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -7368,45 +7368,20 @@ root_key_sentinel_return_servfail(query_ctx_t *qctx, isc_result_t result) {
  */
 static bool
 query_usestale(query_ctx_t *qctx) {
-	bool staleanswersok = false;
-	dns_ttl_t stale_ttl = 0;
-	isc_result_t result;
-
 	qctx_clean(qctx);
 	qctx_freedata(qctx);
 
-	/*
-	 * Stale answers only make sense if stale_ttl > 0 but we want rndc to
-	 * be able to control returning stale answers if they are configured.
-	 */
-	dns_db_attach(qctx->client->view->cachedb, &qctx->db);
-	result = dns_db_getservestalettl(qctx->db, &stale_ttl);
-	if (result == ISC_R_SUCCESS && stale_ttl > 0) {
-		switch (qctx->client->view->staleanswersok) {
-		case dns_stale_answer_yes:
-			staleanswersok = true;
-			break;
-		case dns_stale_answer_conf:
-			staleanswersok = qctx->client->view->staleanswersenable;
-			break;
-		case dns_stale_answer_no:
-			staleanswersok = false;
-			break;
-		}
-	} else {
-		staleanswersok = false;
-	}
-
-	if (staleanswersok) {
+	if (dns_view_staleanswerenabled(qctx->client->view)) {
+		dns_db_attach(qctx->client->view->cachedb, &qctx->db);
 		qctx->client->query.dboptions |= DNS_DBFIND_STALEOK;
 		if (qctx->client->query.fetch != NULL) {
 			dns_resolver_destroyfetch(&qctx->client->query.fetch);
 		}
-	} else {
-		dns_db_detach(&qctx->db);
+
+		return (true);
 	}
 
-	return (staleanswersok);
+	return (false);
 }
 
 /*%

From 966060c03b757eb57f6c9a35ba3fd81dab390690 Mon Sep 17 00:00:00 2001
From: Diego Fronza <diego@isc.org>
Date: Thu, 14 Jan 2021 17:44:19 -0300
Subject: [PATCH 11/13] Extracted common function from query_lookup and
 query_refresh_rrset

Both functions employed the same code lines to allocate query context
buffers, which are used to store query results, so this shared portion
of code was extracted out to a new function, qctx_prepare_buffers.

Also, this commit uses qctx_init to initialize the query context whitin
query_refresh_rrset function.
---
 lib/ns/query.c | 223 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 138 insertions(+), 85 deletions(-)

diff --git a/lib/ns/query.c b/lib/ns/query.c
index b452068f1a..0184ccf2bc 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -5122,6 +5122,26 @@ qctx_init(ns_client_t *client, dns_fetchevent_t **eventp, dns_rdatatype_t qtype,
 	CALL_HOOK_NORETURN(NS_QUERY_QCTX_INITIALIZED, qctx);
 }
 
+/*
+ * Make 'dst' and exact copy of 'src', with exception of the
+ * option field, which is reset to zero.
+ * This function also attaches dst's view and db to the src's
+ * view and cachedb.
+ */
+static void
+qctx_copy(const query_ctx_t *qctx, query_ctx_t *dst) {
+	REQUIRE(qctx != NULL);
+	REQUIRE(dst != NULL);
+
+	memmove(dst, qctx, sizeof(*dst));
+	dst->view = NULL;
+	dst->db = NULL;
+	dst->options = 0;
+	dns_view_attach(qctx->view, &dst->view);
+	dns_db_attach(qctx->view->cachedb, &dst->db);
+	CCTRACE(ISC_LOG_DEBUG(3), "qctx_copy");
+}
+
 /*%
  * Clean up and disassociate the rdataset and node pointers in qctx.
  */
@@ -5602,6 +5622,66 @@ cleanup:
 	return (result);
 }
 
+/*
+ * Allocate buffers in 'qctx' used to store query results.
+ *
+ * 'buffer' must be a pointer to an object whose lifetime
+ * doesn't expire while 'qctx' is in use.
+ */
+static isc_result_t
+qctx_prepare_buffers(query_ctx_t *qctx, isc_buffer_t *buffer) {
+	REQUIRE(qctx != NULL);
+	REQUIRE(qctx->client != NULL);
+	REQUIRE(buffer != NULL);
+
+	qctx->dbuf = ns_client_getnamebuf(qctx->client);
+	if (ISC_UNLIKELY(qctx->dbuf == NULL)) {
+		CCTRACE(ISC_LOG_ERROR,
+			"qctx_prepare_buffers: ns_client_getnamebuf "
+			"failed");
+		return (ISC_R_NOMEMORY);
+	}
+
+	qctx->fname = ns_client_newname(qctx->client, qctx->dbuf, buffer);
+	if (ISC_UNLIKELY(qctx->fname == NULL)) {
+		CCTRACE(ISC_LOG_ERROR,
+			"qctx_prepare_buffers: ns_client_newname failed");
+
+		return (ISC_R_NOMEMORY);
+	}
+
+	qctx->rdataset = ns_client_newrdataset(qctx->client);
+	if (ISC_UNLIKELY(qctx->rdataset == NULL)) {
+		CCTRACE(ISC_LOG_ERROR,
+			"qctx_prepare_buffers: ns_client_newrdataset failed");
+		goto error;
+	}
+
+	if ((WANTDNSSEC(qctx->client) || qctx->findcoveringnsec) &&
+	    (!qctx->is_zone || dns_db_issecure(qctx->db)))
+	{
+		qctx->sigrdataset = ns_client_newrdataset(qctx->client);
+		if (qctx->sigrdataset == NULL) {
+			CCTRACE(ISC_LOG_ERROR,
+				"qctx_prepare_buffers: "
+				"ns_client_newrdataset failed (2)");
+			goto error;
+		}
+	}
+
+	return (ISC_R_SUCCESS);
+
+error:
+	if (qctx->fname != NULL) {
+		ns_client_releasename(qctx->client, &qctx->fname);
+	}
+	if (qctx->rdataset != NULL) {
+		ns_client_putrdataset(qctx->client, &qctx->rdataset);
+	}
+
+	return (ISC_R_NOMEMORY);
+}
+
 /*
  * Setup a new query context for resolving a query.
  *
@@ -5614,71 +5694,40 @@ cleanup:
  * after answering a client with stale data.
  */
 static void
-query_refresh_rrset(query_ctx_t *qctx) {
-	isc_buffer_t b;
-	query_ctx_t new_qctx;
+query_refresh_rrset(query_ctx_t *orig_qctx) {
+	isc_buffer_t buffer;
+	query_ctx_t qctx;
 
-	memset(&new_qctx, 0, sizeof(new_qctx));
+	REQUIRE(orig_qctx != NULL);
+	REQUIRE(orig_qctx->client != NULL);
 
-	dns_view_attach(qctx->view, &new_qctx.view);
-
-	new_qctx.qtype = new_qctx.type = qctx->qtype;
-	new_qctx.dns64 = qctx->dns64;
-	new_qctx.dns64_exclude = qctx->dns64_exclude;
-	new_qctx.client = qctx->client;
-	new_qctx.client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
+	qctx_copy(orig_qctx, &qctx);
+	qctx.client->query.dboptions &= ~(DNS_DBFIND_STALEONLY |
+					  DNS_DBFIND_STALEOK |
+					  DNS_DBFIND_STALEENABLED);
 
 	/*
-	 * If it's a SIG query, we'll iterate the node.
+	 * We'll need some resources...
 	 */
-	if (qctx->qtype == dns_rdatatype_rrsig ||
-	    qctx->qtype == dns_rdatatype_sig) {
-		new_qctx.type = dns_rdatatype_any;
+	if (qctx_prepare_buffers(&qctx, &buffer) != ISC_R_SUCCESS) {
+		dns_db_detach(&qctx.db);
+		qctx_destroy(&qctx);
+		return;
 	}
 
-	new_qctx.dbuf = ns_client_getnamebuf(qctx->client);
-	if (ISC_UNLIKELY(new_qctx.dbuf == NULL)) {
-		CCTRACE(ISC_LOG_ERROR,
-			"query_refresh_rrset: ns_client_getnamebuf "
-			"failed");
-		goto done;
+	/*
+	 * Pretend we didn't find anything in cache.
+	 */
+	(void)query_gotanswer(&qctx, ISC_R_NOTFOUND);
+
+	if (qctx.fname != NULL) {
+		ns_client_releasename(qctx.client, &qctx.fname);
+	}
+	if (qctx.rdataset != NULL) {
+		ns_client_putrdataset(qctx.client, &qctx.rdataset);
 	}
 
-	new_qctx.fname = ns_client_newname(new_qctx.client, new_qctx.dbuf, &b);
-	new_qctx.rdataset = ns_client_newrdataset(qctx->client);
-
-	if (ISC_UNLIKELY(new_qctx.fname == NULL || new_qctx.rdataset == NULL)) {
-		CCTRACE(ISC_LOG_ERROR,
-			"query_refresh_rrset: ns_client_newname failed");
-
-		goto done;
-	}
-
-	if ((WANTDNSSEC(qctx->client) || qctx->findcoveringnsec) &&
-	    (!qctx->is_zone || dns_db_issecure(qctx->db)))
-	{
-		new_qctx.sigrdataset = ns_client_newrdataset(qctx->client);
-		if (new_qctx.sigrdataset == NULL) {
-			CCTRACE(ISC_LOG_ERROR, "query_refresh_rrset: "
-					       "ns_client_newrdataset failed ");
-			goto done;
-		}
-	}
-
-	new_qctx.options = qctx->options;
-	new_qctx.result = ISC_R_SUCCESS;
-	new_qctx.findcoveringnsec = qctx->view->synthfromdnssec;
-
-	(void)query_gotanswer(&new_qctx, ISC_R_NOTFOUND);
-
-done:
-	if (new_qctx.fname != NULL) {
-		ns_client_releasename(new_qctx.client, &new_qctx.fname);
-	}
-	if (new_qctx.rdataset != NULL) {
-		ns_client_putrdataset(new_qctx.client, &new_qctx.rdataset);
-	}
-	qctx_destroy(&new_qctx);
+	qctx_destroy(&qctx);
 }
 
 /*%
@@ -5688,7 +5737,7 @@ done:
  */
 static isc_result_t
 query_lookup(query_ctx_t *qctx) {
-	isc_buffer_t b;
+	isc_buffer_t buffer;
 	isc_result_t result = ISC_R_UNSET;
 	dns_clientinfomethods_t cm;
 	dns_clientinfo_t ci;
@@ -5709,37 +5758,12 @@ query_lookup(query_ctx_t *qctx) {
 	/*
 	 * We'll need some resources...
 	 */
-	qctx->dbuf = ns_client_getnamebuf(qctx->client);
-	if (ISC_UNLIKELY(qctx->dbuf == NULL)) {
-		CCTRACE(ISC_LOG_ERROR, "query_lookup: ns_client_getnamebuf "
-				       "failed (2)");
-		QUERY_ERROR(qctx, ISC_R_NOMEMORY);
+	result = qctx_prepare_buffers(qctx, &buffer);
+	if (result != ISC_R_SUCCESS) {
+		QUERY_ERROR(qctx, result);
 		return (ns_query_done(qctx));
 	}
 
-	qctx->fname = ns_client_newname(qctx->client, qctx->dbuf, &b);
-	qctx->rdataset = ns_client_newrdataset(qctx->client);
-
-	if (ISC_UNLIKELY(qctx->fname == NULL || qctx->rdataset == NULL)) {
-		CCTRACE(ISC_LOG_ERROR, "query_lookup: ns_client_newname failed "
-				       "(2)");
-		QUERY_ERROR(qctx, ISC_R_NOMEMORY);
-		return (ns_query_done(qctx));
-	}
-
-	if ((WANTDNSSEC(qctx->client) || qctx->findcoveringnsec) &&
-	    (!qctx->is_zone || dns_db_issecure(qctx->db)))
-	{
-		qctx->sigrdataset = ns_client_newrdataset(qctx->client);
-		if (qctx->sigrdataset == NULL) {
-			CCTRACE(ISC_LOG_ERROR, "query_lookup: "
-					       "ns_client_newrdataset failed "
-					       "(2)");
-			QUERY_ERROR(qctx, ISC_R_NOMEMORY);
-			return (ns_query_done(qctx));
-		}
-	}
-
 	/*
 	 * Now look for an answer in the database.
 	 */
@@ -5797,6 +5821,35 @@ query_lookup(query_ctx_t *qctx) {
 		dns_cache_updatestats(qctx->view->cache, result);
 	}
 
+	/*
+	 * Special case handling, when stale-answer-client-timeout >= 0 and
+	 * stale answers are enabled, we do not want to return a stale NXRRSET
+	 * entry in cache during the initial lookup or a subsequent lookup when
+	 * stale-answer-client-timeout triggers, instead, BIND must attempt to
+	 * refresh the RRset.
+	 * It is fine to return such entry if resolver-query-timeout has
+	 * triggered, in that case DNS_DBFIND_STALEONLY will not be set during
+	 * the lookup.
+	 */
+	if (result == DNS_R_NCACHENXRRSET &&
+	    ((dboptions & DNS_DBFIND_STALEONLY) != 0) && STALE(qctx->rdataset))
+	{
+		if (qctx->node != NULL) {
+			dns_db_detachnode(qctx->db, &qctx->node);
+		}
+		qctx_freedata(qctx);
+		if ((qctx->options & DNS_GETDB_STALEFIRST) != 0) {
+			/*
+			 * stale-answer-client-timeout is zero and we found a
+			 * stale NXRRSET entry in cache during the first lookup.
+			 * BIND must attempt to refresh the RRset instead of
+			 * using it in this case.
+			 */
+			query_refresh_rrset(qctx);
+		}
+		return (result);
+	}
+
 	/*
 	 * If DNS_DBFIND_STALEOK is set this means we are dealing with a
 	 * lookup following a failed lookup and it is okay to serve a stale

From fa0c9280d23a1e17654e51060aaafa282bd66b50 Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Wed, 20 Jan 2021 16:13:49 +0100
Subject: [PATCH 12/13] Update code flow in query.c wrt stale data

First of all, there was a flaw in the code related to the
'stale-refresh-time' option. If stale answers are enabled, and we
returned stale data, then it was assumed that it was because we were
in the 'stale-refresh-time' window. But now we could also have returned
stale data because of a 'stale-answer-client-timeout'. To fix this,
introduce a rdataset attribute DNS_RDATASETATTR_STALE_WINDOW to
indicate whether the stale cache entry was returned because the
'stale-refresh-time' window is active.

Second, remove the special case handling when the result is
DNS_R_NCACHENXRRSET. This can be done more generic in the code block
when dealing with stale data.

Putting all stale case handling in the code block when dealing with
stale data makes the code more easy to follow.

Update documentation to be more verbose and to match then new code
flow.
---
 lib/dns/include/dns/rdataset.h |   1 +
 lib/dns/rbtdb.c                |  27 +++-
 lib/ns/query.c                 | 260 ++++++++++++++++++---------------
 3 files changed, 160 insertions(+), 128 deletions(-)

diff --git a/lib/dns/include/dns/rdataset.h b/lib/dns/include/dns/rdataset.h
index 72d1445649..9f493e191c 100644
--- a/lib/dns/include/dns/rdataset.h
+++ b/lib/dns/include/dns/rdataset.h
@@ -189,6 +189,7 @@ struct dns_rdataset {
 #define DNS_RDATASETATTR_CYCLIC	      0x00800000 /*%< Cyclic ordering. */
 #define DNS_RDATASETATTR_STALE	      0x01000000
 #define DNS_RDATASETATTR_ANCIENT      0x02000000
+#define DNS_RDATASETATTR_STALE_WINDOW 0x04000000
 
 /*%
  * _OMITDNSSEC:
diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c
index 24442bd3ef..13be34268f 100644
--- a/lib/dns/rbtdb.c
+++ b/lib/dns/rbtdb.c
@@ -273,7 +273,8 @@ typedef ISC_LIST(dns_rbtnode_t) rbtnodelist_t;
 #define RDATASET_ATTR_ZEROTTL	     0x0800
 #define RDATASET_ATTR_CASEFULLYLOWER 0x1000
 /*%< Ancient - awaiting cleanup. */
-#define RDATASET_ATTR_ANCIENT 0x2000
+#define RDATASET_ATTR_ANCIENT	   0x2000
+#define RDATASET_ATTR_STALE_WINDOW 0x4000
 
 /*
  * XXX
@@ -303,6 +304,9 @@ typedef ISC_LIST(dns_rbtnode_t) rbtnodelist_t;
 #define STALE(header)                                                          \
 	((atomic_load_acquire(&(header)->attributes) & RDATASET_ATTR_STALE) != \
 	 0)
+#define STALE_WINDOW(header)                           \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  RDATASET_ATTR_STALE_WINDOW) != 0)
 #define RESIGN(header)                                 \
 	((atomic_load_acquire(&(header)->attributes) & \
 	  RDATASET_ATTR_RESIGN) != 0)
@@ -3148,6 +3152,9 @@ bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdatasetheader_t *header,
 		rdataset->attributes |= DNS_RDATASETATTR_PREFETCH;
 	}
 	if (STALE(header)) {
+		if (STALE_WINDOW(header)) {
+			rdataset->attributes |= DNS_RDATASETATTR_STALE_WINDOW;
+		}
 		rdataset->attributes |= DNS_RDATASETATTR_STALE;
 		rdataset->stale_ttl =
 			(rbtdb->serve_stale_ttl + header->rdh_ttl) - now;
@@ -4551,6 +4558,8 @@ check_stale_header(dns_rbtnode_t *node, rdatasetheader_t *header,
 		 * skip this record.  We skip the records with ZEROTTL
 		 * (these records should not be cached anyway).
 		 */
+
+		RDATASET_ATTR_CLR(header, RDATASET_ATTR_STALE_WINDOW);
 		if (!ZEROTTL(header) && KEEPSTALE(search->rbtdb) &&
 		    stale > search->now) {
 			mark_header_stale(search->rbtdb, header);
@@ -4562,13 +4571,6 @@ check_stale_header(dns_rbtnode_t *node, rdatasetheader_t *header,
 			 */
 			if ((search->options & DNS_DBFIND_STALEOK) != 0) {
 				header->last_refresh_fail_ts = search->now;
-			} else if ((search->options & DNS_DBFIND_STALEONLY) !=
-				   0) {
-				/*
-				 * We want stale RRset only, so we don't skip
-				 * it.
-				 */
-				return (false);
 			} else if ((search->options &
 				    DNS_DBFIND_STALEENABLED) != 0 &&
 				   search->now <
@@ -4581,6 +4583,15 @@ check_stale_header(dns_rbtnode_t *node, rdatasetheader_t *header,
 				 * then don't skip this stale entry but use it
 				 * instead.
 				 */
+				RDATASET_ATTR_SET(header,
+						  RDATASET_ATTR_STALE_WINDOW);
+				return (false);
+			} else if ((search->options & DNS_DBFIND_STALEONLY) !=
+				   0) {
+				/*
+				 * We want stale RRset only, so we don't skip
+				 * it.
+				 */
 				return (false);
 			}
 			return ((search->options & DNS_DBFIND_STALEOK) == 0);
diff --git a/lib/ns/query.c b/lib/ns/query.c
index 0184ccf2bc..f413ddee19 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -145,6 +145,9 @@
 /*% Does the rdataset 'r' contain a stale answer? */
 #define STALE(r) (((r)->attributes & DNS_RDATASETATTR_STALE) != 0)
 
+/*% Does the rdataset 'r' is stale and within stale-refresh-time? */
+#define STALE_WINDOW(r) (((r)->attributes & DNS_RDATASETATTR_STALE_WINDOW) != 0)
+
 #ifdef WANT_QUERYTRACE
 static inline void
 client_trace(ns_client_t *client, int level, const char *message) {
@@ -5745,8 +5748,10 @@ query_lookup(query_ctx_t *qctx) {
 	unsigned int dboptions;
 	dns_ttl_t stale_refresh = 0;
 	bool dbfind_stale = false;
-	bool stale_ok;
-	bool stale_used = false;
+	bool stale_only = false;
+	bool stale_found = false;
+	bool refresh_rrset = false;
+	bool stale_refresh_window = false;
 
 	CCTRACE(ISC_LOG_DEBUG(3), "query_lookup");
 
@@ -5775,14 +5780,10 @@ query_lookup(query_ctx_t *qctx) {
 
 	if ((qctx->options & DNS_GETDB_STALEFIRST) != 0) {
 		/*
-		 * If DNS_GETDB_STALEFIRST is set, it means that stale
-		 * data may be returned as part of this lookup.
-		 * An attempt to refresh the RRset will still take
-		 * place if either an active RRset isn't available or
-		 * a stale one was found.
-		 * This is the expected behavior when
-		 * stale-answer-client-timeout value is zero and stale
-		 * answers are enabled.
+		 * If DNS_GETDB_STALEFIRST is set, it means that a stale
+		 * RRset may be returned as part of this lookup. An attempt
+		 * to refresh the RRset will still take place if an
+		 * active RRset is not available.
 		 */
 		qctx->client->query.dboptions |= DNS_DBFIND_STALEONLY;
 	}
@@ -5821,52 +5822,45 @@ query_lookup(query_ctx_t *qctx) {
 		dns_cache_updatestats(qctx->view->cache, result);
 	}
 
-	/*
-	 * Special case handling, when stale-answer-client-timeout >= 0 and
-	 * stale answers are enabled, we do not want to return a stale NXRRSET
-	 * entry in cache during the initial lookup or a subsequent lookup when
-	 * stale-answer-client-timeout triggers, instead, BIND must attempt to
-	 * refresh the RRset.
-	 * It is fine to return such entry if resolver-query-timeout has
-	 * triggered, in that case DNS_DBFIND_STALEONLY will not be set during
-	 * the lookup.
-	 */
-	if (result == DNS_R_NCACHENXRRSET &&
-	    ((dboptions & DNS_DBFIND_STALEONLY) != 0) && STALE(qctx->rdataset))
-	{
-		if (qctx->node != NULL) {
-			dns_db_detachnode(qctx->db, &qctx->node);
-		}
-		qctx_freedata(qctx);
-		if ((qctx->options & DNS_GETDB_STALEFIRST) != 0) {
-			/*
-			 * stale-answer-client-timeout is zero and we found a
-			 * stale NXRRSET entry in cache during the first lookup.
-			 * BIND must attempt to refresh the RRset instead of
-			 * using it in this case.
-			 */
-			query_refresh_rrset(qctx);
-		}
-		return (result);
-	}
-
 	/*
 	 * If DNS_DBFIND_STALEOK is set this means we are dealing with a
 	 * lookup following a failed lookup and it is okay to serve a stale
-	 * answer. This will start a time window in rbtdb, tracking the last
-	 * time the RRset lookup failed.
-	 *
-	 * A stale answer may also be served if this is a normal lookup,
-	 * the view has enabled serve-stale (DNS_DBFIND_STALE_ENABLED is set),
-	 * and the request is within the stale-refresh-time window. If this
-	 * is the case we have to make sure that the lookup found a stale
-	 * answer, otherwise "fresh" answers are also treated as stale.
+	 * answer. This will (re)start the 'stale-refresh-time' window in
+	 * rbtdb, tracking the last time the RRset lookup failed.
 	 */
 	dbfind_stale = ((dboptions & DNS_DBFIND_STALEOK) != 0);
-	stale_ok = ((dboptions &
-		     (DNS_DBFIND_STALEENABLED | DNS_DBFIND_STALEONLY)) != 0);
 
-	if (dbfind_stale || (stale_ok && STALE(qctx->rdataset))) {
+	/*
+	 * If DNS_DBFIND_STALEENABLED is set, this may be a normal lookup, but
+	 * we are allowed to immediately respond with a stale answer if the
+	 * request is within the 'stale-refresh-time' window.
+	 */
+	stale_refresh_window = (STALE_WINDOW(qctx->rdataset) &&
+				(dboptions & DNS_DBFIND_STALEENABLED) != 0);
+
+	/*
+	 * If DNS_DBFIND_STALEONLY is set, a stale positive answer is requested.
+	 * This can happen if 'stale-answer-client-timeout' is enabled.
+	 *
+	 * If 'stale-answer-client-timeout' is set to 0, and a stale positive
+	 * answer is found, send it to the client, and try to refresh the
+	 * RRset. If a stale negative answer is found, continue with recursion
+	 * (perhaps the query will be resolved eventually and the answer from
+	 * the authoritative is returned to the client, or the query will
+	 * timeout, in that case DNS_DBFIND_STALEOK may be set, and a stale
+	 * negative answer is returned (or SERVFAIL).
+	 *
+	 * If 'stale-answer-client-timeout' is non-zero, and a stale positive
+	 * answer is found, send it to the client. Don't try to refresh the
+	 * RRset because a fetch is already in progress. If a stale negative
+	 * answer is found, then abort the lookup and the client has to wait
+	 * until recursion is finished.
+	 */
+	stale_only = ((dboptions & DNS_DBFIND_STALEONLY) != 0);
+
+	if (dbfind_stale ||
+	    (STALE(qctx->rdataset) && (stale_only || stale_refresh_window)))
+	{
 		char namebuf[DNS_NAME_FORMATSIZE];
 
 		inc_stats(qctx->client, ns_statscounter_trystale);
@@ -5876,106 +5870,132 @@ query_lookup(query_ctx_t *qctx) {
 		    STALE(qctx->rdataset))
 		{
 			qctx->rdataset->ttl = qctx->view->staleanswerttl;
-			stale_used = true;
+			stale_found = true;
 		} else {
-			stale_used = false;
+			stale_found = false;
 		}
 
 		dns_name_format(qctx->client->query.qname, namebuf,
 				sizeof(namebuf));
+
 		if (dbfind_stale) {
 			isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
 				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
 				      "%s resolver failure, stale answer %s",
 				      namebuf,
-				      stale_used ? "used" : "unavailable");
-		} else if ((qctx->options & DNS_GETDB_STALEFIRST) != 0 &&
-			   stale_used) {
-			isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
-				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
-				      "%s stale answer used, an attempt to "
-				      "refresh the RRset will still be made",
-				      namebuf);
-		} else if ((dboptions & DNS_DBFIND_STALEONLY) != 0) {
-			isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
-				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
-				      "%s client timeout, stale answer %s",
-				      namebuf,
-				      stale_used ? "used" : "unavailable");
-		} else {
+				      stale_found ? "used" : "unavailable");
+			if (!stale_found) {
+				/*
+				 * Resolver failure, no stale data, nothing
+				 * more we can do, return SERVFAIL.
+				 */
+				QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+				return (ns_query_done(qctx));
+			}
+
+			stale_only = false;
+		} else if (stale_refresh_window) {
+			/*
+			 * A recent lookup failed, so during this time window
+			 * we are allowed to return stale data immediately.
+			 */
 			isc_log_write(ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
 				      NS_LOGMODULE_QUERY, ISC_LOG_INFO,
 				      "%s query within stale refresh time, "
 				      "stale answer %s",
 				      namebuf,
-				      stale_used ? "used" : "unavailable");
-		}
+				      stale_found ? "used" : "unavailable");
 
-		if (!stale_used &&
-		    ((qctx->options & DNS_GETDB_STALEFIRST) == 0)) {
-			/*
-			 * At this point, we know that stale data was not
-			 * available. A fetch may still be in progress to
-			 * add the data in cache, but if DNS_DBFIND_STALEONLY
-			 * is set, that means the client timeout was triggered.
-			 * But no answer was found, so we need to wait for the
-			 * original query to be resumed. If no client timeout
-			 * is active, then we have completed the fetch, or it
-			 * timed out, and we are done with the query.
-			 */
-			if ((dboptions & DNS_DBFIND_STALEONLY) == 0) {
+			if (!stale_found) {
+				/*
+				 * During the stale refresh window explicitly
+				 * do not try to refresh the data, because a
+				 * recent lookup failed.
+				 */
 				QUERY_ERROR(qctx, DNS_R_SERVFAIL);
 				return (ns_query_done(qctx));
 			}
 
-			return (result);
+			stale_only = false;
+		} else if ((dboptions & DNS_DBFIND_STALEONLY) != 0) {
+			if ((qctx->options & DNS_GETDB_STALEFIRST) != 0) {
+				if (stale_found && result == ISC_R_SUCCESS) {
+					/*
+					 * Immediately return the stale answer,
+					 * start a resolver fetch to refresh
+					 * the data in cache.
+					 */
+					isc_log_write(
+						ns_lctx,
+						NS_LOGCATEGORY_SERVE_STALE,
+						NS_LOGMODULE_QUERY,
+						ISC_LOG_INFO,
+						"%s stale answer used, an "
+						"attempt to refresh the RRset "
+						"will still be made",
+						namebuf);
+					refresh_rrset = true;
+				} else {
+					/*
+					 * We have nothing useful in cache to
+					 * return immediately.
+					 */
+					qctx_clean(qctx);
+					qctx_freedata(qctx);
+					dns_db_attach(
+						qctx->client->view->cachedb,
+						&qctx->db);
+					qctx->client->query.dboptions &=
+						~DNS_DBFIND_STALEONLY;
+					qctx->options &= ~DNS_GETDB_STALEFIRST;
+					if (qctx->client->query.fetch != NULL) {
+						dns_resolver_destroyfetch(
+							&qctx->client->query
+								 .fetch);
+					}
+					return query_lookup(qctx);
+				}
+			} else {
+				/*
+				 * The 'stale-answer-client-timeout' triggered,
+				 * return the stale answer if available,
+				 * otherwise wait until the resolver finishes.
+				 */
+				isc_log_write(
+					ns_lctx, NS_LOGCATEGORY_SERVE_STALE,
+					NS_LOGMODULE_QUERY, ISC_LOG_INFO,
+					"%s client timeout, stale answer %s",
+					namebuf,
+					stale_found ? "used" : "unavailable");
+				if (!stale_found || result != ISC_R_SUCCESS) {
+					return (result);
+				}
+			}
 		}
 	} else {
+		stale_only = false;
+	}
+
+	if (!stale_only) {
 		/*
-		 * If we are here then either no stale RRset was found
-		 * or this is a regular query whose expected result is an
-		 * active RRset, ensure this flag won't affect further
-		 * database operations by disabling it.
+		 * The DNS_DBFIND_STALEONLY option may be set, but if we
+		 * didn't have stale data in cache, or we responded with
+		 * a stale answer because of 'stale-refresh-time', this is
+		 * actually not a 'stale-only' lookup. Clear the flag to
+		 * allow the client to be detach the handle.
 		 */
 		qctx->client->query.dboptions &= ~DNS_DBFIND_STALEONLY;
 	}
 
-	/*
-	 * If DNS_DBFIND_STALEONLY is disabled then we proceed as normal,
-	 * otherwise we only proceed with query_gotanswer if we
-	 * successfully found a stale RRset in cache, since this is an
-	 * attempt to find stale data after stale-answer-client-timeout
-	 * timer has expired. If no stale data was found then we must allow
-	 * a running fetch to complete in order to properly update the RRset.
-	 *
-	 * We must also ensure that if DNS_GETDB_STALEFIRST is set we won't
-	 * skip a call to query_gotanswer if we failed to find stale data,
-	 * since this means stale-answer-client-timeout is zero and we only
-	 * want to return stale data if any is available, otherwise we want
-	 * to resolve the query using the standard resolution process.
-	 */
-	if (result != ISC_R_SUCCESS &&
-	    ((dboptions & DNS_DBFIND_STALEONLY) != 0) &&
-	    ((qctx->options & DNS_GETDB_STALEFIRST) == 0))
-	{
-		goto cleanup;
-	}
-
 	result = query_gotanswer(qctx, result);
-	stale_ok = (qctx->options & DNS_GETDB_STALEFIRST) != 0;
 
-	qctx->client->query.dboptions &= ~DNS_DBFIND_STALEOK;
-
-	if (stale_used && stale_ok) {
+	if (refresh_rrset) {
 		/*
-		 * If we reached this point then it means that we've
-		 * found a stale RRset entry in cache and BIND is
-		 * configured to allow queries to be answered with
-		 * stale data if no active RRset is available,
-		 * i.e. "stale-anwer-client-timeout 0".
-		 * We still need to refresh the RRset, a call to
-		 * query_refresh_rrset() is made to create a new query context
-		 * for that purpose.
+		 * If we reached this point then it means that we have found a
+		 * stale RRset entry in cache and BIND is configured to allow
+		 * queries to be answered with stale data if no active RRset
+		 * is available, i.e. "stale-anwer-client-timeout 0". But, we
+		 * still need to refresh the RRset.
 		 */
 		query_refresh_rrset(qctx);
 	}

From d8c6655d7dd01171d977ac7993d911c9108a813d Mon Sep 17 00:00:00 2001
From: Matthijs Mekking <matthijs@isc.org>
Date: Fri, 22 Jan 2021 13:33:22 +0100
Subject: [PATCH 13/13] Rewrap comments to 80 char width serve-stale test

---
 bin/tests/system/serve-stale/tests.sh | 77 +++++++++++++--------------
 1 file changed, 37 insertions(+), 40 deletions(-)

diff --git a/bin/tests/system/serve-stale/tests.sh b/bin/tests/system/serve-stale/tests.sh
index 8e009c3839..92263d86df 100755
--- a/bin/tests/system/serve-stale/tests.sh
+++ b/bin/tests/system/serve-stale/tests.sh
@@ -547,8 +547,8 @@ grep "ANSWER: 0," dig.out.test$n > /dev/null || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status+ret))
 
-# keep track of time so we can access these rrset later,
-# when we expect them to become ancient.
+# Keep track of time so we can access these RRset later, when we expect them
+# to become ancient.
 t1=`$PERL -e 'print time()'`
 
 n=$((n+1))
@@ -643,16 +643,16 @@ grep "1 #NXDOMAIN" ns1/named.stats.$n.cachedb > /dev/null || ret=1
 status=$((status+ret))
 if [ $ret != 0 ]; then echo_i "failed"; fi
 
-# retrieve max-stale-ttl value,
+# Retrieve max-stale-ttl value.
 interval_to_ancient=`grep 'max-stale-ttl' ns1/named2.conf.in  | awk '{ print $2 }' | tr -d ';'`
-# we add 2 seconds to it since this is the ttl value of the records being tested.
+# We add 2 seconds to it since this is the ttl value of the records being
+# tested.
 interval_to_ancient=$((interval_to_ancient + 2))
 t2=`$PERL -e 'print time()'`
 elapsed=$((t2 - t1))
 
-# if elapsed time so far is less than max-stale-ttl + 2 seconds,
-# then we sleep enough to ensure that we'll ask for ancient rrsets
-# in the next queries.
+# If elapsed time so far is less than max-stale-ttl + 2 seconds, then we sleep
+# enough to ensure that we'll ask for ancient RRsets in the next queries.
 if [ $elapsed -lt $interval_to_ancient ]; then
     sleep $((interval_to_ancient - elapsed))
 fi
@@ -1039,8 +1039,8 @@ $RNDCCMD 10.53.0.3 stats > /dev/null 2>&1
 [ -f ns3/named.stats ] || ret=1
 cp ns3/named.stats ns3/named.stats.$n
 # Check first 10 lines of Cache DB statistics. After last queries, we expect
-# one active TXT RRset, one stale TXT, one stale nxrrset TXT, and one
-# stale NXDOMAIN.
+# one active TXT RRset, one stale TXT, one stale nxrrset TXT, and one stale
+# NXDOMAIN.
 grep -A 10 "++ Cache DB RRsets ++" ns3/named.stats.$n > ns3/named.stats.$n.cachedb || ret=1
 grep "1 TXT" ns3/named.stats.$n.cachedb > /dev/null || ret=1
 grep "1 #TXT" ns3/named.stats.$n.cachedb > /dev/null || ret=1
@@ -1259,8 +1259,8 @@ $RNDCCMD 10.53.0.4 stats > /dev/null 2>&1
 [ -f ns4/named.stats ] || ret=1
 cp ns4/named.stats ns4/named.stats.$n
 # Check first 10 lines of Cache DB statistics. After last queries, we expect
-# one active TXT RRset, one stale TXT, one stale nxrrset TXT, and one
-# stale NXDOMAIN.
+# one active TXT RRset, one stale TXT, one stale nxrrset TXT, and one stale
+# NXDOMAIN.
 grep -A 10 "++ Cache DB RRsets ++" ns4/named.stats.$n > ns4/named.stats.$n.cachedb || ret=1
 grep "1 TXT" ns4/named.stats.$n.cachedb > /dev/null || ret=1
 grep "1 #TXT" ns4/named.stats.$n.cachedb > /dev/null || ret=1
@@ -1281,9 +1281,9 @@ status=$((status+ret))
 echo_i "stop ns4"
 $PERL ../stop.pl --use-rndc --port ${CONTROLPORT} serve-stale ns4
 
-# Load the cache as if it was five minutes (RBTDB_VIRTUAL) older.
-# Since max-stale-ttl defaults to a week, we need to adjust the date by
-# one week and five minutes.
+# Load the cache as if it was five minutes (RBTDB_VIRTUAL) older. Since
+# max-stale-ttl defaults to a week, we need to adjust the date by one week and
+# five minutes.
 LASTWEEK=`TZ=UTC perl -e 'my $now = time();
         my $oneWeekAgo = $now - 604800;
         my $fiveMinutesAgo = $oneWeekAgo - 300;
@@ -1640,21 +1640,19 @@ $DIG -p ${PORT} +tries=1 +timeout=3  @10.53.0.3 nodata.example TXT > dig.out.tes
 $DIG -p ${PORT} +tries=1 +timeout=30  @10.53.0.3 nodata.example TXT > dig.out.test$((n+2))
 wait
 
-# Since nodata.example is cached as NXRRSET and marked as
-# stale at this point, BIND must not return this RRset when
-# stale-answer-client-timeout triggers, instead, it must attempt
-# to refresh the RRset.
-# Since the authoritative server is disabled and we are using
-# resolver-query-timeout value of 10 seconds, we expect this
-# query with a timeout of 3 seconds to time out.
+# Since nodata.example is cached as NXRRSET and marked as stale at this point,
+# BIND must not return this RRset when stale-answer-client-timeout triggers,
+# instead, it must attempt to refresh the RRset. Since the authoritative
+# server is disabled and we are using resolver-query-timeout value of 10
+# seconds, we expect this query with a timeout of 3 seconds to time out.
 n=$((n+1))
 echo_i "check query for nodata.example times out (default stale-answer-client-timeout) ($n)"
 grep "connection timed out" dig.out.test$n > /dev/null || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status+ret))
 
-# For this query we expect BIND to return stale NXRRSET data
-# for nodata.example after resolver-query-timeout expires.
+# For this query we expect BIND to return stale NXRRSET data for
+# nodata.example after resolver-query-timeout expires.
 n=$((n+1))
 echo_i "check stale nodata.example comes from cache after resolver-query-timeout expires (default stale-answer-client-timeout) ($n)"
 grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
@@ -1682,10 +1680,10 @@ rndc_reload ns3 10.53.0.3
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status+ret))
 
-# Send a query, auth server is disabled, we will enable it after
-# a while in order to receive an answer before resolver-query-timeout
-# expires. Since stale-answer-client-timeout is disabled we must receive
-# an answer from authoritative server.
+# Send a query, auth server is disabled, we will enable it after a while in
+# order to receive an answer before resolver-query-timeout expires. Since
+# stale-answer-client-timeout is disabled we must receive an answer from
+# authoritative server.
 echo_i "sending query for test $((n+2))"
 $DIG -p ${PORT} @10.53.0.3 data.example TXT > dig.out.test$((n+2)) &
 sleep 3
@@ -1783,8 +1781,8 @@ wait_for_rrset_refresh() {
 }
 
 # This test ensures that after we get stale data due to
-# stale-answer-client-timeout 0, enabling the authoritative server
-# will allow the RRset to be updated.
+# stale-answer-client-timeout 0, enabling the authoritative server will allow
+# the RRset to be updated.
 n=$((n+1))
 ret=0
 echo_i "check stale data.example was refreshed (stale-answer-client-timeout 0) ($n)"
@@ -1810,21 +1808,20 @@ $DIG -p ${PORT} +tries=1 +timeout=3  @10.53.0.3 nodata.example TXT > dig.out.tes
 $DIG -p ${PORT} +tries=1 +timeout=30  @10.53.0.3 nodata.example TXT > dig.out.test$((n+2))
 wait
 
-# Since nodata.example is cached as NXRRSET and marked as
-# stale at this point, BIND must not prompty return this RRset
-# due to stale-answer-client-timeout == 0, instead, it must
-# attempt to refresh the RRset.
-# Since the authoritative server is disabled and we are using
-# resolver-query-timeout value of 10 seconds, we expect this
-# query with a timeout of 3 seconds to time out.
+# Since nodata.example is cached as NXRRSET and marked as stale at this point,
+# BIND must not prompty return this RRset due to
+# stale-answer-client-timeout == 0, instead, it must attempt to refresh the
+# RRset. Since the authoritative server is disabled and we are using
+# resolver-query-timeout value of 10 seconds, we expect this query with a
+# timeout of 3 seconds to time out.
 n=$((n+1))
 echo_i "check query for nodata.example times out (stale-answer-client-timeout 0) ($n)"
 grep "connection timed out" dig.out.test$n > /dev/null || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status+ret))
 
-# For this query we expect BIND to return stale NXRRSET data
-# for nodata.example after resolver-query-timeout expires.
+# For this query we expect BIND to return stale NXRRSET data for
+# nodata.example after resolver-query-timeout expires.
 n=$((n+1))
 echo_i "check stale nodata.example comes from cache after resolver-query-timeout expires (stale-answer-client-timeout 0) ($n)"
 grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
@@ -1906,8 +1903,8 @@ if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status+ret))
 
 # This test ensures that after we get stale data due to
-# stale-answer-client-timeout 0, enabling the authoritative server
-# will allow the RRset to be updated.
+# stale-answer-client-timeout 0, enabling the authoritative server will allow
+# the RRset to be updated.
 n=$((n+1))
 ret=0
 echo_i "check stale data.example was refreshed (stale-answer-client-timeout 0 stale-refresh-time 4) ($n)"