Stop serving stale responses from negative cache.

[gonzalop]

Serving stale responses from the negative cache interacts poorly with
other plugins and seems to confuse users.

Fixes #3586.

1. Why is this pull request needed and what does it do?

See issue #3586

2. Which issues (if any) are related?

#3586

3. Which documentation changes (if any) need to be made?

None.

4. Does this introduce a backward incompatible change or deprecation?

No.

[codecov-io]

Codecov Report

Merging #3736 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #3736      +/-   ##
=========================================
+ Coverage   56.77%   56.8%   +0.02%     
=========================================
  Files         220     220              
  Lines       11071   11071              
=========================================
+ Hits         6286    6289       +3     
+ Misses       4303    4300       -3     
  Partials      482     482

Impacted Files	Coverage Δ
plugin/cache/handler.go	92.95% <100%> (+7.04%)	⬆️
plugin/errors/errors.go	95.23% <0%> (-4.77%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a64f0c7...1fa10a7. Read the comment docs.

[chrisohaver]

Should getIgnoreTTL check the positive cache for an unexpired entry before returning a stale negative cache entry?

[gonzalop]

It is the same logic present in get, but if you prefer it that way, I'll flip the order.

[chrisohaver]

It is the same logic present in get, but if you prefer it that way, I'll flip the order.

No, not exactly. I'm not suggesting to simply flip the order (I agree, that would be pointless).

I'm suggesting ...

1. if a fresh neg-cache entry exists return it.
2. if a stale neg-cache entry exists check pos-cache for fresh entry:
   • if a fresh pos-cache entry exists, return pos-cache entry
   • if no fresh pos-cache entry exists, return the stale neg-cache entry
3. if the neg-cache entry is rotten, or does not exist, check pos-cache entry.
4. if pos-cache entry exists and is not rotten, return it

This would prevent a stale neg-cache entry from masking a fresh pos-cache entry (e.g. put in place by the stale retrieve prefetch).

fresh/stale/rotten definitions:
fresh = unexpired cache entry
stale = expired cache entry within stale threshold
rotten = expired cache entry past stale threshold

[chrisohaver]

See #3744 for alternate fix.

[gonzalop]

Closing in favor of chris' fix.
Thanks!