doc: Explain vcl_refresh_* better

as suggested by Dridi here: #4400 (comment)

Author: nigoroll

doc/sphinx/reference/vcl_step.rst

@@ -345,6 +345,8 @@ the stale ones.
 The returned value only affects response headers, the body that is delivered
 is always the one from the stale object.
 
+See :ref:`vcl-built-in-refresh` for additional explanations.
+
   |
   | ``merge``
   |  Merge the headers we got from the backend response with

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

@@ -70,6 +70,107 @@ This granularity, and the general goal of the built-in subroutines split
 is to allow to circumvent a specific aspect of the default rules without
 giving the entire logic up.
 
+Specific split built-in subroutines
+-----------------------------------
+
+Some split subroutines in the built-in VCL deserve additional explanations:
+
+.. _vcl-built-in-refresh:
+
+vcl_refresh_*
+~~~~~~~~~~~~~
+
+These subroutines handle edge cases of backend refreshes. The precondition for
+these to be entered implicitly or explicitly via ``vcl_backend_refresh`` is that
+the current backend request can potentially create a cache object (that is, it
+is not for a private object as created by a pass or hit-for-pass) and that a
+stale object was found in cache which is not already invalidated. If this is the
+case, core code constructs a conditional ``GET`` request with the
+``If-Modified-Since`` and/or ``If-None-Match`` headers set before
+``vcl_backend_fetch`` is entered. If the VCL code does not remove the headers,
+the backend might respond with a ``304 Not Modified`` status, in which case
+``vcl_backend_refresh`` is called on the response to decide what do do (see
+:ref:`vcl_backend_refresh` for reference) and, if the built-in VCL is reached,
+the subs documented below will be called via ``vcl_builtin_backend_refresh``.
+
+vcl_refresh_valid
+~~~~~~~~~~~~~~~~~
+
+``vcl_refresh_valid`` handles the case where the stale object to be revalidated
+by the 304 response got explicitly removed from the cache by a ban or purge
+while the backend request was in progress::
+
+	sub vcl_refresh_valid {
+		if (!obj_stale.is_valid) {
+			return (error(503, "Invalid object for refresh"));
+		}
+	}
+
+The error is generated because alternative actions might require additional
+consideration. There are basically two options:
+
+We can ignore the fact that the now successfully revalidated object was *just*
+invalidated by not falling through to the built-in VCL with this subroutine in
+the user VCL::
+
+	sub vcl_refresh_valid {
+		return;
+	}
+
+This avoids the error but can potentially result in invalidations being
+ineffective.
+
+The other option is to retry the backend request without the conditional request
+headers. This option is implicitly active whenever the user VCL results in a
+``return(retry)`` from ``vcl_backend_error``, because core code removes the
+conditional request headers if the stale object is found to be invalidated.
+
+A variant of this option is an explicit retry for the case at hand::
+
+	sub vcl_refresh_valid {
+                return (retry);
+        }
+
+To summarize, refreshes should work fine as long as there is at least one retry
+from ``vcl_backend_error`` for 503 errors. Additionally, VCL allows for
+customization if needed.
+
+vcl_refresh_conditions
+~~~~~~~~~~~~~~~~~~~~~~
+
+This sub safeguards against invalid 304 responses getting unnoticed::
+
+	sub vcl_refresh_conditions {
+		if (!bereq.http.if-modified-since &&
+		    !bereq.http.if-none-match) {
+			return (error(503, "Unexpected 304"));
+		}
+	}
+
+A backend should not respond with a 304 if neither of the conditional request
+headers were present in the backend request.
+
+vcl_refresh_status
+~~~~~~~~~~~~~~~~~~
+
+This sub safeguards against accidental 304 responses if the stale object does
+not have a 200 status::
+
+	sub vcl_refresh_status {
+		if (obj_stale.status != 200) {
+			return (error(503, "Invalid object for refresh (status)"));
+		}
+	}
+
+The background here is that the HTTP standards only allow refreshes of status
+200 objects, but Vinyl Cache core code allows to deliberately violate this. In
+such cases, the status check needs to be neutered by not running the built-in
+code using::
+
+	sub vcl_refresh_status {
+                return;
+        }
+
 Built-in VCL reference
 ----------------------