Add the internal bo->failed state we need to allow retries separate

from the external BOS_FAILED state.

bin/varnishd/cache/cache.h

@@ -500,6 +500,9 @@ oc_getlru(const struct objcore *oc)
  * streaming delivery will make use of.
  */
 
+/*
+ * The macro-states we expose outside the fetch code
+ */
 enum busyobj_state_e {
 	BOS_INVALID = 0,	/* don't touch (yet) */
 	BOS_REQ_DONE,		/* beresp.* can be examined */
@@ -514,7 +517,6 @@ struct busyobj {
 	struct lock		mtx;
 	pthread_cond_t		cond;
 	char			*end;
-	enum fetch_step		step;
 
 	/*
 	 * All fields from refcount and down are zeroed when the busyobj
@@ -532,6 +534,7 @@ struct busyobj {
 	intptr_t		vfps_priv[N_VFPS];
 	int			vfp_nxt;
 
+	int			failed;
 	enum busyobj_state_e	state;
 
 	struct ws		ws[1];

bin/varnishd/cache/cache_fetch.c

@@ -494,7 +494,7 @@ vbf_stp_fetch(struct worker *wrk, struct busyobj *bo)
 	if (vbf_beresp2obj(bo)) {
 		(void)VFP_Error(bo, "Could not get storage");
 		VDI_CloseFd(&bo->vbc);
-		return (F_STP_DONE);
+		return (F_STP_ERROR);
 	}
 
 	assert(WRW_IsReleased(wrk));
@@ -526,7 +526,7 @@ vbf_stp_fetch(struct worker *wrk, struct busyobj *bo)
 	VSLb(bo->vsl, SLT_Fetch_Body, "%u(%s)",
 	    bo->htc.body_status, body_status_2str(bo->htc.body_status));
 
-	if (bo->state == BOS_FAILED) {
+	if (bo->failed) {
 		wrk->stats.fetch_failed++;
 	} else {
 		if (bo->do_stream)
@@ -553,16 +553,29 @@ vbf_stp_fetch(struct worker *wrk, struct busyobj *bo)
 		}
 	}
 
-	if (!bo->do_stream && bo->state != BOS_FAILED)
+	if (!bo->do_stream) {
+		if (bo->failed) {
+			if (bo->fetch_obj != NULL) {
+				oc_freeobj(bo->fetch_objcore);
+				bo->fetch_obj = NULL;
+				bo->stats->n_object--;
+			}
+			return (F_STP_ERROR);
+		} else {
 			HSH_Unbusy(&wrk->stats, obj->objcore);
+			VBO_setstate(bo, BOS_FINISHED);
+		}
+	} else if (bo->failed) {
+		HSH_Fail(bo->fetch_objcore);
+		VBO_setstate(bo, BOS_FAILED);
+	} else {
+		VBO_setstate(bo, BOS_FINISHED);
+	}
 
 	HSH_Complete(obj->objcore);
 
 	assert(bo->refcount >= 1);
 
-	if (bo->state != BOS_FAILED)
-		VBO_setstate(bo, BOS_FINISHED);
-
 	return (F_STP_DONE);
 }
 
@@ -650,14 +663,16 @@ vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo)
 			if (st->len == st->space)
 				st = NULL;
 		}
-	} while (bo->state < BOS_FAILED &&
-	    (ois == OIS_DATA || ois == OIS_STREAM));
+	} while (!bo->failed && (ois == OIS_DATA || ois == OIS_STREAM));
 	ObjIterEnd(&oi);
-	if (bo->state != BOS_FAILED) {
+	if (!bo->failed) {
 		assert(al == bo->ims_obj->len);
 		assert(obj->len == al);
 		VBO_setstate(bo, BOS_FINISHED);
 		EXP_Rearm(bo->ims_obj, bo->ims_obj->exp.t_origin, 0, 0, 0);
+	} else {
+		HSH_Fail(bo->fetch_objcore);
+		VBO_setstate(bo, BOS_FAILED);
 	}
 	HSH_Complete(obj->objcore);
 	return (F_STP_DONE);
@@ -673,7 +688,7 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 	CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
 
-	AN(bo->fetch_objcore->flags &= OC_F_BUSY);
+	AN(bo->fetch_objcore->flags & OC_F_BUSY);
 
 	// XXX: reset all beresp flags ?
 
@@ -697,7 +712,9 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 	http_PrintfHeader(bo->beresp, "X-XXXPHK: yes");
 
 	if (vbf_beresp2obj(bo)) {
-		INCOMPL();
+		VBO_setstate(bo, BOS_FAILED);
+		HSH_Fail(bo->fetch_objcore);
+		return (F_STP_DONE);
 	}
 
 	HSH_Unbusy(&wrk->stats, bo->fetch_obj->objcore);
@@ -739,7 +756,6 @@ vbf_fetch_thread(struct worker *wrk, void *priv)
 
 	while (stp != F_STP_DONE) {
 		CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
-		bo->step = stp;
 		switch(stp) {
 #define FETCH_STEP(l, U, arg)						\
 		case F_STP_##U:						\

bin/varnishd/cache/cache_fetch_proc.c

@@ -62,12 +62,11 @@ VFP_Error(struct busyobj *bo, const char *fmt, ...)
 
 	CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
 	assert(bo->state >= BOS_REQ_DONE);
-	if (bo->state < BOS_FAILED) {
+	if (!bo->failed) {
 		va_start(ap, fmt);
 		VSLbv(bo->vsl, SLT_FetchError, fmt, ap);
 		va_end(ap);
-		HSH_Fail(bo->fetch_objcore);
-		VBO_setstate(bo, BOS_FAILED);
+		bo->failed = 1;
 	}
 	return (VFP_ERROR);
 }
@@ -214,7 +213,7 @@ VFP_Fetch_Body(struct busyobj *bo, ssize_t est)
 			bo->should_close = 1;
 			break;
 		}
-		assert(bo->state != BOS_FAILED);
+		AZ(bo->failed);
 		if (st == NULL) {
 			st = VFP_GetStorage(bo, est);
 			est = 0;
@@ -228,7 +227,7 @@ VFP_Fetch_Body(struct busyobj *bo, ssize_t est)
 		CHECK_OBJ_NOTNULL(st, STORAGE_MAGIC);
 		assert(st == VTAILQ_LAST(&bo->fetch_obj->store, storagehead));
 		l = st->space - st->len;
-		assert(bo->state != BOS_FAILED);
+		AZ(bo->failed);
 		vfps = VFP_Suck(bo, st->ptr + st->len, &l);
 		if (l > 0 && vfps != VFP_ERROR) {
 			assert(!VTAILQ_EMPTY(&bo->fetch_obj->store));
@@ -239,7 +238,7 @@ VFP_Fetch_Body(struct busyobj *bo, ssize_t est)
 	} while (vfps == VFP_OK);
 
 	if (vfps == VFP_ERROR) {
-		assert(bo->state == BOS_FAILED);
+		AN(bo->failed);
 		(void)VFP_Error(bo, "Fetch Pipeline failed to process");
 		bo->should_close = 1;
 	}

bin/varnishtest/tests/r01284.vtc

@@ -19,6 +19,10 @@ varnish v1 \
 	}
 } -start
 
+varnish v1 -cliok "param.set debug +syncvsl"
+
+delay .1
+
 client c1 {
 	# Fill transient
 	txreq -url "/obj1"
@@ -26,6 +30,8 @@ client c1 {
 	expect resp.status == 200
 } -run
 
+delay .1
+
 varnish v1 -expect SMA.Transient.g_bytes > 1048000
 varnish v1 -expect SMA.Transient.g_space < 200
 
@@ -36,8 +42,8 @@ client c1 {
 	delay 1
 } -run
 
-# Two failures, one for obj2 and one for the attempts at sending error
-varnish v1 -expect SMA.Transient.c_fail == 2
+# Three failures, one for obj2, one for vcl_backend_error{} and for vcl_error{}
+varnish v1 -expect SMA.Transient.c_fail == 3
 
 client c1 {
 	# Check that Varnish is still alive