condfetch: Skip revalidation of an invalidated stale_oc

[dridi]

If we can catch a dying stale_oc before initiating a condfetch, we can fall back to a regular fetch instead. If we catch it too late, when we are ready to merge headers, we transition to vcl_backend_error and get a chance to retry.

This avoids a race between a condfetch and the invalidation of its stale objcore, covered with a purge in test cases.

[dridi]

@nigoroll @bsdphk: I think the first commit should be merged before the release.

[dridi]

@hermunn brought an interesting point regarding bans. We check the stale object against newer bans during lookup, but in the absence of another lookup or if the ban lurker doesn't get a chance to evaluate post-lookup bans (and the default ban_lurker_age almost guarantees that) the stale object will not be invalidated.

To get the discussion started I pushed what could look like evaluating bans before and after 304 revalidation. I will turn this pull request into a draft.

[nigoroll]

I understand that Dridi prepared these patches in a rush, so once the dust has settled, I would appreciate better explanations in the commit messages.

Regarding the actual issue here, I am not convinced yet that failing with a fetch error for the race is the best option (it might be, but I would like to think about it). There will probably be many sites for which the current behavior is just fine, and who would start to see errors unless they added the return(retry), so we should at least add this to the builtin vcl and docs.

If someone issues a PURGE, they expect that the respective object go away and not survive indirectly through a revalidation. The current suggestion to fail to v_b_e {} and then retry under VCL control has the advantage of replacing the busy object and thus not delivering the corrupted stale content. But we could also just accept the revalidated object and deliver it (once), also implicitly purging it. Or we could do the retry internally without VCL involvement.

Again, not sure yet.

[dridi]

bugwash:

• go back to the original scope of the pull request
  • leave post-lookup ban checks out for now
• add 304 retry in built-in vcl_backend_error
• docs

[dridi]

Updated as per bugwash consensus.

[nigoroll]

This looks really good after cleanup.

[dridi]

@AlveElde brought up an interesting point in an offline discussion. An implicit retry from the built-in VCL is not desirable since there is no guarantee that vcl_backend_fetch is idempotent.

I added at the beginning of this patch series two commits that:

• split the startfetch step in two (prepfetch, then what remains for startfetch)
• introduce return (retry(fetch)) and return (retry(task)) transitions

The return (retry(task)) one is present for completeness and is otherwise equivalent to return (retry). In all cases a retry increments the retries counter and is limited by the max_retries parameter.

This allows an effective retry at the actual step where we initiate the backend request, skipping vcl_backend_fetch and staying in the same transaction. The commit introducing this variation on retries is minimalist, lacking coverage and documentation.

On the other hand, the original commits from this pull request were updated with their documentation and coverage adjusted.

[hermunn]

I have been thinking about this a lot (I have also looked at the code), and I have some opinions:

• Introducing retries in the default VCL is a change very likely cause confusion and unexpected behavior in setups where users already have some complexity in their backend subs. In particular, anyone writing if (bereq.retries > 0) might have assumptions baked in about why the retry happens (in their VCL), and they did not think about this other posibility. The fact that sub vcl_backend_fetch is not run twice (something we need coverage on, for sure) makes it better, but the fact that you can have a bereq.retries > 0 in sub vcl_backend_response after going only once through sub vcl_backend_fetch is a gotcha, and needs to be documented carefully. It might be better to have two variables you can check in VCL, one which includes these short false starts, and one which does not.
• When a backend responds with 304, it is correct and reasonable to evaluate new bans against the stale_oc. Am also convinced that the right Varnish behavior is to refuse to deliver that banned stale object,with no way to override this from VCL.
  It is right to drop the stale_oc reference and go to sub vcl_backend_error, like this PR does. The conundrum for me, is what Varnish and the default VCL should do when this unfortunate situation, a banned stale OC with nothing to send to the client, happens.

Maybe it is better to insist that the user ask (through VCL in sub vcl_backend_fetch, or through a parameter) for the immediate retry sans IMS headers without taking a detour through sub vcl_backend_error. This will make the change to the Varnish Final state less dramatic, but I am not sure about our users' needs in this area.

In other words, we would leave return (retry) as it is now, allowing users to, if they really need it, do relatively complex logic for this case, while most users simply will opt in or out of the immediate retry. Of course, Varnish would remove the IMS headers at the right moment, probably before going back to sub backend_fetch, while the default VCL would simply serve a 503.

[dridi]

I agree with the points you raise. I still think that an explicit retry(fetch) action would be very useful outside of the built-in VCL to enable retries without tearing down privs for the task, and without going through a potentially-non-idempotent vcl_backend_fetch.

It might be better to have two variables you can check in VCL, one which includes these short false starts, and one which does not.

Or maybe make this opt-in, without a detour via vcl_backend_error?

sub vcl_backend_fetch {
        set bereq.retry_dying_304 = true;
}

No rug pull, no PRIV_TASK loss, no required attention to vcl_backend_fetch idempotency.

[hermunn]

I think the new VCL variable retry_dying_304 is a good idea. We might also consider documenting that from September, this will/might be default true, so that everyone should make sure to set this in their own configuration (VCL) if they care.

Also, I see your point with return (retry(fetch)) in general, as many users simply want to set a new backend and retry without any sub vcl_backend_fetch logic being relevant. However, if we go with the retry_dying_304 here, the question about return (retry(fetch)) might be better discussed in a different PR.

[dridi]

I'm wondering to which extent we could actually tie this to vcl_backend_refresh instead of vcl_backend_error, having this return (retry(fetch)) in the built-in VCL once we move some of the core code handling 304s to VCL.

See #3994 for more details.

That would make the bereq.retry_dying_304 idea irrelevant.

edit: and this could not qualify as a breaking change in a new subroutine for which there is no preexisting VCL.