From 8ed6d3d2dd4ff6b0dbd785dcccff7f4b5497cc5d Mon Sep 17 00:00:00 2001
From: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>
Date: Tue, 19 Mar 2024 11:43:17 +0100
Subject: [PATCH] fetch: Retire waiting list safety net

If a synthetic beresp is inserted into the cache with no TTL, grace or
keep at all, it is now considered validated (although not in the best
circumstances) and will be passed to all requests in the waiting list.
---
 bin/varnishd/cache/cache_fetch.c  | 24 ++---------
 bin/varnishtest/tests/c00131.vtc  | 71 +++++++++++++++++++++++++++++++
 doc/sphinx/reference/vcl_step.rst | 21 ++++-----
 3 files changed, 83 insertions(+), 33 deletions(-)
 create mode 100644 bin/varnishtest/tests/c00131.vtc

diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 6df7d546d61..172d8fc687a 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -896,9 +896,7 @@ vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo)
  *
  * replaces a stale object unless
  * - abandoning the bereq or
- * - leaving vcl_backend_error with return (deliver) and beresp.ttl == 0s or
- * - there is a waitinglist on this object because in this case the default ttl
- *   would be 1s, so we might be looking at the same case as the previous
+ * - leaving vcl_backend_error with return (deliver)
  *
  * We do want the stale replacement to avoid an object pileup with short ttl and
  * long grace/keep, yet there could exist cases where a cache object is
@@ -954,23 +952,9 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 
 	stale = bo->stale_oc;
 	oc->t_origin = now;
-	if (!VTAILQ_EMPTY(&oc->objhead->waitinglist)) {
-		/*
-		 * If there is a waitinglist, it means that there is no
-		 * grace-able object, so cache the error return for a
-		 * short time, so the waiting list can drain, rather than
-		 * each objcore on the waiting list sequentially attempt
-		 * to fetch from the backend.
-		 */
-		oc->ttl = 1;
-		oc->grace = 5;
-		oc->keep = 5;
-		stale = NULL;
-	} else {
-		oc->ttl = 0;
-		oc->grace = 0;
-		oc->keep = 0;
-	}
+	oc->ttl = 0;
+	oc->grace = 0;
+	oc->keep = 0;
 
 	synth_body = VSB_new_auto();
 	AN(synth_body);
diff --git a/bin/varnishtest/tests/c00131.vtc b/bin/varnishtest/tests/c00131.vtc
new file mode 100644
index 00000000000..c9ce19bd588
--- /dev/null
+++ b/bin/varnishtest/tests/c00131.vtc
@@ -0,0 +1,71 @@
+varnishtest "waiting list rush from vcl_backend_error"
+
+barrier b1 sock 2
+barrier b2 sock 2
+
+server s1 {
+	rxreq
+	send garbage
+} -start
+
+varnish v1 -cliok "param.set debug +syncvsl,+waitinglist"
+varnish v1 -vcl+backend {
+	import vtc;
+	sub vcl_backend_fetch {
+		vtc.barrier_sync("${b1_sock}");
+		vtc.barrier_sync("${b2_sock}");
+	}
+	sub vcl_backend_error {
+		set beresp.http.error-vxid = bereq.xid;
+	}
+} -start
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.http.error-vxid == 1002
+} -start
+
+barrier b1 sync
+
+logexpect l1 -v v1 -q Debug -g raw {
+	loop 4 {
+		expect * * Debug "on waiting list"
+	}
+} -start
+
+client c2 {
+	txreq
+	rxresp
+	expect resp.http.error-vxid == 1002
+} -start
+
+client c3 {
+	txreq
+	rxresp
+	expect resp.http.error-vxid == 1002
+} -start
+
+client c4 {
+	txreq
+	rxresp
+	expect resp.http.error-vxid == 1002
+} -start
+
+client c5 {
+	txreq
+	rxresp
+	expect resp.http.error-vxid == 1002
+} -start
+
+logexpect l1 -wait
+
+barrier b2 sync
+
+client c1 -wait
+client c2 -wait
+client c3 -wait
+client c4 -wait
+client c5 -wait
+
+varnish v1 -expect n_expired == 1
diff --git a/doc/sphinx/reference/vcl_step.rst b/doc/sphinx/reference/vcl_step.rst
index ee07f616923..86fad6312a2 100644
--- a/doc/sphinx/reference/vcl_step.rst
+++ b/doc/sphinx/reference/vcl_step.rst
@@ -378,19 +378,14 @@ This subroutine is called if we fail the backend fetch or if
 
 Returning with :ref:`abandon` does not leave a cache object.
 
-If returning with ``deliver`` and a ``beresp.ttl > 0s``, a synthetic
-cache object is generated in VCL, whose body may be constructed using
-the ``synthetic()`` function.
-
-When there is a waiting list on the object, the default ``ttl`` will
-be positive (currently one second), set before entering
-``vcl_backend_error``. This is to avoid request serialization and
-hammering on a potentially failing backend.
-
-Since these synthetic objects are cached in these special
-circumstances, be cautious with putting private information there. If
-you really must, then you need to explicitly set ``beresp.ttl`` to
-zero in ``vcl_backend_error``.
+If returning with ``deliver`` and ``beresp.uncacheable == false``, a
+synthetic cache object is generated in VCL, whose body may be constructed
+using the ``synthetic()`` function.
+
+Since these synthetic objects are cached in these special circumstances,
+be cautious with putting private information there. If you really must,
+then you need to explicitly set ``beresp.uncacheable`` to ``true`` in
+``vcl_backend_error``.
 
 The `vcl_backend_error` subroutine may terminate with calling ``return()``
 with one of the following keywords: