Repository navigation
fix cache : replay a response that carries Vary only for the matching request headers - #8079
Conversation
|
Correction to my earlier note — I had the mechanism wrong. The Worth recording while we are here: the PR that introduced the regression (#7901) touched Verification for this PR is therefore local:
A follow-up that would actually prevent this class of regression: map |
3b856b2 to
a66575d
Compare
|
Strengthened one test after re-reviewing it: That ordering is the reason this fix works against #7901: Also worth stating explicitly, since it is a trade-off rather than an oversight: the cache key is still |
|
Follow-up opened: #8081 makes |
Aias00
left a comment
There was a problem hiding this comment.
@/tmp/rv8079/review_8079.md
Sync to current master to resolve the stable shenyu-integrated-test-http CI failure (CachePluginTest.testMemoryModle/testRedisModle). Root cause: this branch predated #8079, whose pre-fix CachedResponse.capture() refused to store any response carrying a Vary header, so the cached endpoint was never reused and both requests hit the backend. #8079 keys Vary responses on the selecting request headers instead. Also pulls in the divide/loadbalancer work (#8076, #7172, #7986, #8005, #7821, #8033, #8090). #7172 removeStaleUpstreams reconciles via removeFromMap/triggerRemoveOne, which republish the immutable healthy snapshot under the same lock, so the read-only snapshot model stays correct with no stale retained instances.
Problem
CachedResponse.capturerefuses to store any response that carries aVaryheader:The rejection was added in #7662; since #7901 (
CrossFilter) addsVary: Originto every gateway response, including non-CORS ones, the cache plugin now stores nothing at all.CachePluginTestinshenyu-integrated-test-httpfails on any branch containing both changes (testMemoryModle/testRedisModle:expected: <true> but was: <false>), because the second request is no longer served from the cache.A response that declares
Varyis not uncacheable — it is only reusable for requests that carry the same values for the listed fields. Rejecting it outright trades a correctness rule for a total loss of caching.Change
CachedResponsestores the request header values selected byVary(selectingValues) next to the response, andmatches(request)compares them on lookup. A hit is replayed only when every listed field still has the same values; otherwise the request is a miss and goes upstream.Vary: *is still never stored, since no request can select it.CachePluginapplies the same check on the lookup path and on the single-flight follow path, so a shared fill is never handed to a request that selects another variant.Varyheader lists, and the stored values must be valid header values. Entries written before this change (noselectingValues) still decode, so a rolling upgrade does not invalidate the existing cache.capturenow receives the request in addition to the response.Trade-off worth calling out
The response key is still per
(uri, selector, rule), so a rule serving manyOriginvariants overwrites the entry instead of keeping one entry per variant. Correctness is preserved — a wrong variant is never replayed — only the hit rate suffers for those rules. Keying per variant would require knowing the variant list before the first response is seen; happy to follow up with that if you prefer.Tests
mvn -pl shenyu-plugin/shenyu-plugin-cache/shenyu-plugin-cache-handler test— 100 tests green, checkstyle clean.CacheResponseReplayTest#testVaryHeaderAddedBeforeCommitStillAllowsCachingreproduces the reported failure: aVary: Originadded inbeforeCommit(exactly whatCrossFilterdoes) no longer prevents caching.CacheResponseReplayTest#testResponseVaryingOnRequestHeaderIsReusedOnlyForTheSameVariant: the sameOriginhits, anotherOriginmisses and receives its own body.CachedResponseTest:Vary: *is not stored,selectingValuesmust agree with theVaryheader, legacy entries still decode.Both new replay tests fail without this change (upstream calls 2 instead of 1, and 3 instead of 2).