Repository navigation
[WIP] fix: coalesce() leader fetch always fails due to stale RequestURI, breaking cold-miss coalescing - #4365
Closed
larry-dalmeida wants to merge 2 commits into
Closed
larry-dalmeida wants to merge 2 commits into
larry-dalmeida wants to merge 2 commits into
Conversation
coalesce() clones ctx.Request() (a server-received request, which carries RequestURI and a relative URL) and passes it straight to f.fetch (http.Client.Do). The client unconditionally rejects any request with RequestURI set, so the leader's fetch inside the singleflight closure fails on every single cold miss. coalesce() then returns without calling ctx.Serve(), silently falling through to skipper's normal backend-proxy pipeline instead - which resolves the backend correctly on its own, so client-visible correctness is unaffected and the bug has been invisible. The real effect: singleflight cold-miss coalescing has never actually worked. Concurrent requests to the same cold key each independently fall through to the normal path instead of being deduplicated to one origin request - origin-request amplification under concurrent load. Verified locally: 10 concurrent requests to a fresh cold key produced 10 origin hits before this fix, 1 after. Fix mirrors the already-correct pattern a few hundred lines down in this same file, in doRevalidate's direct-dispatch branch: resolve ctx.BackendUrl(), set req.URL.Scheme/Host, clear req.RequestURI. Fixing only that regressed TestProxy_L1Cache_VaryStar_NeverCached: storeDecision() (coalesce()'s leader path only calls this one, not Response()'s separate storage path) was missing the Vary: * never- cache check Response() already has, and had simply never been exercised before now - coalesce()'s fetch always crashed first. Added the same check to storeDecision(), the one choke point both callers share. Added a regression test using a real httptest backend + proxytest, not a stub: every existing Coalesce* test in this file stubs f.fetch directly, which bypasses net/http's client-side RequestURI validation entirely and cannot catch this class of bug. Confirmed the new test fails (10 backend calls, not 1) with the fix reverted. Pre-existing since the cache() filter's original commit (zalando#3991), through zalando#4033, zalando#4315, zalando#4344 - not introduced by any of those. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
larry-dalmeida
changed the title
Share validated request preparation between cold-miss coalescing and background revalidation. Preserve filtered direct requests and outgoing Host, replay original requests for RFC and unresolved backends, and report invalid backend URLs before pipeline fallback. Use singleflight join barriers for deterministic concurrent tests, cover real-client dispatch and dynamic routing, and prohibit sharing Vary-star responses. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
larry-dalmeida
force-pushed
the
fix/LWSRE-coalesce-requesturi
branch
from
October 22, 2026 14:11
a2c4261 to
ade9b20
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
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.
Note
This PR will be superseded by four smaller, dependent PRs to make review easier:
Please direct further reviews to those PRs, in the order listed above.
Summary
Fixes cold-miss coalescing by preparing inbound requests for
http.Client.Do, so concurrent misses can share one fetch. Responses withVary: *are not cached or shared.End-to-end test
Concurrent requests for one cold cache key go through
proxytestto a realhttptestbackend; the target outcome is successful responses with exactly one backend request.