Merged
Conversation
dridi
reviewed
Sep 24, 2025
nigoroll
force-pushed
the
4399_invalid_refresh
branch
3 times, most recently
from
f35bd5a to
aaa56ef
Compare
dridi
reviewed
Sep 25, 2025
nigoroll
force-pushed
the
4399_invalid_refresh
branch
from
aaa56ef to
9c069fb
Compare
dridi
reviewed
Sep 25, 2025
dridi
reviewed
Sep 25, 2025
Member
Author
|
bugwash: phk ok, I need to work thorugh dridis feedback |
When it is time to decide between making a bereq for a regular or conditional fetch, we need to ensure that the stale object is still valid. There is a window between the lookup and the begining of a fetch task during which the object could have been invalidated, in particular if the fetch task was queued. Catching it early allows to proceed with a regular fetch. Comitter edit: Minor wording change above, because the mkbereq step is not entered for retries. Ref varnishcache#4399
Similar to the previous commit, but we also need to re-check the condition for each backend request because we might have re-tried. Ref varnishcache#4399
We now use a specific reason for the cases of a late fail of a 304 response and also explain how the "wrong status" case can happen and, if intentional, what needs to be done to make it work.
The built-in vcl_refresh_valid returns a 503 error if the stale object used for revalidation gets invalidated while the backend fetch is in progress. One alternative is to not dismiss the stale object, but that can lead to invalidated objects being re-instantiated through concurrent refreshes (varnishcache#4082). This test case covers the other alternative, which is to retry the fetch. We could use sub vcl_refresh_valid { if (!obj_stale.is_valid) { # core code removes conditional headers INM/IMS return (retry); } } but more realisticly, real world VCL will retry in v_b_e, so we take that route and keep the built-in vcl_refresh_valid {} The plain retry works because core code detects the invalidated stale object and removes the conditional headers. Committer edit: Removed code change from original commit, edited test case and, rewrote commit message. See varnishcache#4400 (comment)
as suggested by Dridi here: varnishcache#4400 (comment)
nigoroll
force-pushed
the
4399_invalid_refresh
branch
from
9c069fb to
bb1259f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses the experience documented in #4399: It makes no sense to attempt a conditional GET if we are going to fail it anyway later when
sub vcl_builtin_backend_refresh/sub vcl_refresh_statusrun. In particular, even if the VCL has a retry invcl_backend_error {}, we would attempt the same failing conditional request again.There are three trivial commits:
bereq0.304response and to give guidance on the 304 of non-200 objects use case (which does not conform with http).