varnishcache · dridi · Jun 18, 2024 · Jun 18, 2024 · Mar 12, 2024 · Mar 12, 2024
diff --git a/bin/varnishd/builtin.vcl b/bin/varnishd/builtin.vcl
@@ -257,6 +257,19 @@ sub vcl_backend_error {
 }
 
 sub vcl_builtin_backend_error {
+	call vcl_refresh_error;
+	call vcl_beresp_error;
+}
+
+sub vcl_refresh_error {
+	if (beresp.was_304) {
+		unset bereq.http.If-Modified-Since;
+		unset bereq.http.If-None-Match;
+		return (retry(fetch));
+	}
+}
+
+sub vcl_beresp_error {
 	set beresp.http.Content-Type = "text/html; charset=utf-8";
 	set beresp.http.Retry-After = "5";
 	set beresp.body = {"<!DOCTYPE html>

diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
@@ -42,6 +42,7 @@
 #define FETCH_STEPS \
 	FETCH_STEP(mkbereq,           MKBEREQ) \
 	FETCH_STEP(retry,             RETRY) \
+	FETCH_STEP(prepfetch,         PREPFETCH) \
 	FETCH_STEP(startfetch,        STARTFETCH) \
 	FETCH_STEP(condfetch,         CONDFETCH) \
 	FETCH_STEP(fetch,             FETCH) \
@@ -269,7 +270,7 @@ vbf_stp_mkbereq(struct worker *wrk, struct busyobj *bo)
 	}
 	http_ForceField(bo->bereq0, HTTP_HDR_PROTO, "HTTP/1.1");
 
-	if (bo->stale_oc != NULL &&
+	if (bo->stale_oc != NULL && !(bo->stale_oc->flags & OC_F_DYING) &&
 	    ObjCheckFlag(bo->wrk, bo->stale_oc, OF_IMSCAND) &&
 	    (bo->stale_oc->boc != NULL || ObjGetLen(wrk, bo->stale_oc) != 0)) {
 		AZ(bo->stale_oc->flags & (OC_F_HFM|OC_F_PRIVATE));
@@ -298,7 +299,7 @@ vbf_stp_mkbereq(struct worker *wrk, struct busyobj *bo)
 		bo->req = NULL;
 		ObjSetState(bo->wrk, oc, BOS_REQ_DONE);
 	}
-	return (F_STP_STARTFETCH);
+	return (F_STP_PREPFETCH);
 }
 
 /*--------------------------------------------------------------------
@@ -326,24 +327,30 @@ vbf_stp_retry(struct worker *wrk, struct busyobj *bo)
 	assert(bo->director_state == DIR_S_NULL);
 
 	/* reset other bo attributes - See VBO_GetBusyObj */
-	bo->storage = NULL;
 	bo->do_esi = 0;
 	bo->do_stream = 1;
 	bo->was_304 = 0;
 	bo->err_code = 0;
 	bo->err_reason = NULL;
+	if (bo->htc != NULL)
+		bo->htc->doclose = SC_NULL;
+
+	if (bo->retry_fetch) {
+		bo->retry_fetch = 0;
+		return (F_STP_STARTFETCH);
+	}
+
+	bo->storage = NULL;
 	bo->connect_timeout = NAN;
 	bo->first_byte_timeout = NAN;
 	bo->between_bytes_timeout = NAN;
-	if (bo->htc != NULL)
-		bo->htc->doclose = SC_NULL;
 
 	// XXX: BereqEnd + BereqAcct ?
 	VSL_ChgId(bo->vsl, "bereq", "retry", VXID_Get(wrk, VSL_BACKENDMARKER));
 	VSLb_ts_busyobj(bo, "Start", bo->t_prev);
 	http_VSL_log(bo->bereq);
 
-	return (F_STP_STARTFETCH);
+	return (F_STP_PREPFETCH);
 }
 
 /*--------------------------------------------------------------------
@@ -388,17 +395,11 @@ vbf_304_logic(struct busyobj *bo)
  */
 
 static const struct fetch_step * v_matchproto_(vbf_state_f)
-vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
+vbf_stp_prepfetch(struct worker *wrk, struct busyobj *bo)
 {
-	int i;
-	vtim_real now;
-	unsigned handling;
-	struct objcore *oc;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 	CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
-	oc = bo->fetch_objcore;
-	CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
 
 	AZ(bo->storage);
 	bo->storage = bo->uncacheable ? stv_transient : STV_next();
@@ -416,6 +417,25 @@ vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
 
 	assert (wrk->vpi->handling == VCL_RET_FETCH ||
 	    wrk->vpi->handling == VCL_RET_ERROR);
+	return (F_STP_STARTFETCH);
+}
+
+/*--------------------------------------------------------------------
+ * Send bereq, fetch beresp, run vcl_backend_response
+ */
+
+static const struct fetch_step * v_matchproto_(vbf_state_f)
+vbf_stp_startfetch(struct worker *wrk, struct busyobj *bo)
+{
+	struct objcore *oc;
+	unsigned handling;
+	vtim_real now;
+	int i;
+
+	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
+	CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
+	oc = bo->fetch_objcore;
+	CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
 
 	HTTP_Setup(bo->beresp, bo->ws, bo->vsl, SLT_BerespMethod);
 
@@ -835,6 +855,13 @@ vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo)
 	stale_oc = bo->stale_oc;
 	CHECK_OBJ_NOTNULL(stale_oc, OBJCORE_MAGIC);
 
+	if (stale_oc->flags & OC_F_DYING) {
+		(void)VFP_Error(bo->vfc, "Template object invalidated");
+		vbf_cleanup(bo);
+		wrk->stats->fetch_failed++;
+		return (F_STP_ERROR);
+	}
+
 	stale_boc = HSH_RefBoc(stale_oc);
 	CHECK_OBJ_ORNULL(stale_boc, BOC_MAGIC);
 	if (stale_boc) {

diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c
@@ -637,6 +637,16 @@ VRT_SetHdr(VRT_CTX, VCL_HEADER hs, const char *pfx, VCL_STRANDS s)
 
 /*--------------------------------------------------------------------*/
 
+void
+VPI_retry(VRT_CTX, unsigned is_task)
+{
+
+	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+	CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC);
+	ctx->bo->retry_fetch = !is_task;
+	VRT_handling(ctx, VCL_RET_RETRY);
+}
+
 VCL_VOID
 VRT_handling(VRT_CTX, unsigned hand)
 {

diff --git a/bin/varnishtest/tests/c00129.vtc b/bin/varnishtest/tests/c00129.vtc
@@ -0,0 +1,58 @@
+varnishtest "304 on invalidated stale object"
+
+barrier b1 cond 2
+barrier b2 cond 2
+
+server s1 {
+	rxreq
+	txresp -hdr {ETag: "foo"} -body corrupted
+
+	rxreq
+	expect req.http.If-None-Match == {"foo"}
+	barrier b1 sync
+	barrier b2 sync
+	txresp -status 304 -hdr {ETag: "foo"}
+
+	rxreq
+	expect req.http.If-None-Match == <undef>
+	txresp -hdr {ETag: "foo"} -body valid
+} -start
+
+varnish v1 -vcl+backend {
+	sub vcl_recv {
+		if (req.method == "PURGE") {
+			return (purge);
+		}
+	}
+	sub vcl_backend_response {
+		set beresp.ttl = 1ms;
+		set beresp.grace = 0s;
+		set beresp.keep = 10s;
+	}
+	sub vcl_deliver {
+		set resp.http.obj-hits = obj.hits;
+	}
+} -start
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.body == corrupted
+	expect resp.http.obj-hits == 0
+
+	delay 0.1
+
+	txreq
+	rxresp
+	expect resp.body == valid
+	expect resp.http.obj-hits == 0
+} -start
+
+barrier b1 sync
+client c2 {
+	txreq -req PURGE
+	rxresp
+} -run
+barrier b2 sync
+
+client c1 -wait
diff --git a/bin/varnishtest/tests/c00130.vtc b/bin/varnishtest/tests/c00130.vtc
@@ -0,0 +1,59 @@
+varnishtest "Demote condfetch to fetch on invalidated stale object"
+
+barrier b1 sock 2
+barrier b2 sock 2
+
+server s1 {
+	rxreq
+	txresp -hdr {ETag: "foo"} -body corrupted
+
+	rxreq
+	expect req.http.If-None-Match == <undef>
+	txresp -hdr {ETag: "foo"} -body valid
+} -start
+
+varnish v1 -vcl+backend {
+	import vtc;
+	sub vcl_recv {
+		if (req.method == "PURGE") {
+			return (purge);
+		}
+	}
+	sub vcl_miss {
+		if (req.http.sync) {
+			vtc.barrier_sync("${b1_sock}");
+			vtc.barrier_sync("${b2_sock}");
+		}
+	}
+	sub vcl_backend_response {
+		set beresp.ttl = 1ms;
+		set beresp.grace = 0s;
+		set beresp.keep = 10s;
+	}
+	sub vcl_deliver {
+		set resp.http.obj-hits = obj.hits;
+	}
+} -start
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.body == corrupted
+	expect resp.http.obj-hits == 0
+
+	delay 0.1
+
+	txreq -hdr "sync: true"
+	rxresp
+	expect resp.body == valid
+	expect resp.http.obj-hits == 0
+} -start
+
+barrier b1 sync
+client c2 {
+	txreq -req PURGE
+	rxresp
+} -run
+barrier b2 sync
+
+client c1 -wait
diff --git a/doc/sphinx/reference/vcl_step.rst b/doc/sphinx/reference/vcl_step.rst
@@ -392,6 +392,10 @@ 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 a conditional fetch failed to process a 304 response and transitioned
+to ``vcl_backend_error``, the backend request is retried for a regular
+fetch within the same transaction.
+
 The `vcl_backend_error` subroutine may terminate with calling ``return()``
 with one of the following keywords:
 

diff --git a/doc/sphinx/reference/vcl_var.rst b/doc/sphinx/reference/vcl_var.rst
@@ -1378,7 +1378,8 @@ beresp.was_304
 
 	When ``true`` this indicates that we got a 304 response
 	to our conditional fetch from the backend and turned
-	that into ``beresp.status = 200``
+	that into ``beresp.status = 200``, unless the refresh
+	attempt failed.
 
 
 obj

diff --git a/include/tbl/bereq_flags.h b/include/tbl/bereq_flags.h
@@ -34,6 +34,7 @@
 /* lower, vcl_r, vcl_w, doc */
 BEREQ_FLAG(uncacheable,	0, 0, "")	// also beresp
 BEREQ_FLAG(is_bgfetch,	1, 0, "")
+BEREQ_FLAG(retry_fetch,	0, 0, "")
 #define REQ_BEREQ_FLAG(lower, vcl_r, vcl_w, doc) \
 	BEREQ_FLAG(lower, vcl_r, vcl_w, doc)
 #include "tbl/req_bereq_flags.h"

diff --git a/include/vcc_interface.h b/include/vcc_interface.h
@@ -40,6 +40,7 @@ void VPI_vcl_rel(VRT_CTX, VCL_VCL);
 void VPI_vcl_select(VRT_CTX, VCL_VCL);
 void v_noreturn_ VPI_Fail(const char *func, const char *file, int line,
     const char *cond);
+void VPI_retry(VRT_CTX, unsigned is_task);
 
 /***********************************************************************
  * VPI_count() refers to this structure for coordinates into the VCL source.

diff --git a/lib/libvcc/vcc_action.c b/lib/libvcc/vcc_action.c
@@ -344,6 +344,27 @@ vcc_act_return_vcl(struct vcc *tl)
 
 /*--------------------------------------------------------------------*/
 
+static void
+vcc_act_return_retry(struct vcc *tl)
+{
+	unsigned is_task = 0;
+
+	ExpectErr(tl, '(');
+	vcc_NextToken(tl);
+	if (vcc_IdIs(tl->t, "task"))
+		is_task = 1;
+	else if (!vcc_IdIs(tl->t, "fetch")) {
+		VSB_printf(tl->sb, "Expected \"task\" or \"fetch\" retry.\n");
+		vcc_ErrWhere(tl, tl->t);
+	}
+	ERRCHK(tl);
+	Fb(tl, 1, "VPI_retry(ctx, %u);\n", is_task);
+	SkipToken(tl, ID);
+	SkipToken(tl, ')');
+}
+
+/*--------------------------------------------------------------------*/
+
 static void v_matchproto_(sym_act_f)
 vcc_act_return(struct vcc *tl, struct token *t, struct symbol *sym)
 {
@@ -388,6 +409,8 @@ vcc_act_return(struct vcc *tl, struct token *t, struct symbol *sym)
 			vcc_act_return_vcl(tl);
 		else if (hand == VCL_RET_PASS)
 			vcc_act_return_pass(tl);
+		else if (hand == VCL_RET_RETRY)
+			vcc_act_return_retry(tl);
 		else if (hand == VCL_RET_FAIL)
 			vcc_act_return_fail(tl);
 		else {