kill stale oc for backend synth and clarify when we cache errors

Fixes #2946 in the sense that we want to treat a cacheable backend synth
like any other object and kill the stale object it replaces.

We do not replace the stale oc when
- abandoning the bereq
- leaving vcl_backend_error with return (deliver) and beresp.ttl == 0s
- 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

Thank you @mbgrydeland for the review and spotting a mistake.

bin/varnishd/cache/cache_fetch.c

@@ -683,6 +683,7 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 	vtim_real now;
 	uint8_t *ptr;
 	struct vsb *synth_body;
+	struct objcore *stale = NULL;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 	CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
@@ -708,6 +709,7 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 	http_TimeHeader(bo->beresp, "Date: ", now);
 	http_SetHeader(bo->beresp, "Server: Varnish");
 
+	stale = bo->stale_oc;
 	bo->fetch_objcore->t_origin = now;
 	if (!VTAILQ_EMPTY(&bo->fetch_objcore->objhead->waitinglist)) {
 		/*
@@ -720,6 +722,7 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 		bo->fetch_objcore->ttl = 1;
 		bo->fetch_objcore->grace = 5;
 		bo->fetch_objcore->keep = 5;
+		stale = NULL;
 	} else {
 		bo->fetch_objcore->ttl = 0;
 		bo->fetch_objcore->grace = 0;
@@ -775,6 +778,8 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 	AZ(ObjSetU64(wrk, bo->fetch_objcore, OA_LEN, o));
 	VSB_destroy(&synth_body);
 	HSH_Unbusy(wrk, bo->fetch_objcore);
+	if (stale != NULL && bo->fetch_objcore->ttl > 0)
+		HSH_Kill(stale);
 	ObjSetState(wrk, bo->fetch_objcore, BOS_FINISHED);
 	return (F_STP_DONE);
 }

bin/varnishtest/tests/r01395.vtc

-varnishtest "Test vcl_synth is called even if vcl_backend_fetch failed"
+varnishtest "Test vcl_backend_error is called if vcl_backend_fetch fails"
 
 varnish v1 -vcl {
 	backend default { .host = "${bad_backend}"; }

bin/varnishtest/tests/r02946.vtc

+varnishtest "#2946 - objcore leak for backend_synth"
+
+varnish v1 -vcl {
+	backend bad { .host = "${bad_backend}"; }
+
+	sub vcl_backend_error {
+		if (bereq.http.abandon) {
+			return (abandon);
+		}
+		if (bereq.http.ttl0) {
+			set beresp.ttl = 0s;
+			return (deliver);
+		}
+		set beresp.status = 200;
+		set beresp.ttl = 0.0001s;
+		set beresp.grace = 1h;
+		return (deliver);
+	}
+} -start
+
+client c1 -repeat 20 -keepalive {
+	txreq
+	rxresp
+	expect resp.status == 200
+
+	delay 0.001
+} -run
+
+delay 2
+
+client c1 {
+	txreq -hdr "abandon: true"
+	rxresp
+	expect resp.status == 200
+	expect resp.http.age > 1
+} -run
+
+client c1 {
+	txreq -hdr "ttl0: true"
+	rxresp
+	expect resp.status == 200
+	expect resp.http.age > 1
+} -run
+
+varnish v1 -expect n_objectcore == 1

doc/sphinx/users-guide/vcl-built-in-subs.rst

@@ -495,8 +495,12 @@ vcl_backend_error
 This subroutine is called if we fail the backend fetch or if
 *max_retries* has been exceeded.
 
-A synthetic object is generated in VCL, whose body may be constructed
-using the ``synthetic()`` function.
+Returning with `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. This may be useful to increase the
+efficiency of failing backend requests.
 
 The `vcl_backend_error` subroutine may terminate with calling ``return()``
 with one of the following keywords: