[master] e12fa0aea fetch: Retire waiting list safety net

[Dridi Boukelmoune]

commit e12fa0aea4cea9ec00556b5ecf6424142cd926f0
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date:   Tue Mar 19 11:43:17 2024 +0100

    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.

diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 8af011fb7..f4f9ee4a1 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -900,9 +900,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
@@ -960,23 +958,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 000000000..c9ce19bd5
--- /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 67ddf0818..81bb7b4e0 100644
--- a/doc/sphinx/reference/vcl_step.rst
+++ b/doc/sphinx/reference/vcl_step.rst
@@ -380,19 +380,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: