Repository navigation
cache: centralize dispatch and preserve response bodies - #4370
Closed
larry-dalmeida wants to merge 7 commits into
Closed
larry-dalmeida wants to merge 7 commits into
larry-dalmeida wants to merge 7 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 (#3991), through #4033, #4315, #4344 - not introduced by any of those. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
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. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Buffer independent nonstoreable follower responses before closing origin streams. Preserve response metadata and count body-read failures as bypass errors without serving incomplete payloads. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Reproduce the cache-first Host and path contract violation through a real proxy, and verify that Host/prefix rewrites before cache preserve cold and warm behavior. Document the required ordering without changing the executor or external deployment routes. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Demonstrate that a response filter after cache is skipped on direct cold, warm, and stale paths. Verify moving it before cache applies client Cache-Control without mutating stored origin directives. Runtime and deployment sources unchanged. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
larry-dalmeida
force-pushed
the
larry-almeida-zse-cache-dispatch-pr
branch
from
October 23, 2026 06:46
3f7646c to
258c6da
Compare
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
|
Docker image "registry-write.opensource.zalan.do/teapot/skipper:d8f1d2e9be3439ffcd91eeba5f0bf80c79403578" is not based on an approved base image. Any production deployment relying on this image will be blocked. To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Zalando Container Registry namespace |
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
1 of 2 tasks
|
Docker image "registry-write.opensource.zalan.do/teapot/skipper:064c34b21d23000eaab4ee3e009059861e95740b" is not based on an approved base image. Any production deployment relying on this image will be blocked. To create a compliant Docker image of your application, you must reference an allowed Docker image as its base image in your Dockerfile. This base image must come from the Zalando Container Registry namespace |
Collaborator
Author
|
Superseded by #4394. Changes consolidated into five focused commits with published e2e evidence. |
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.
Summary of changes
Share request preparation between cold fetches and revalidation, preserving backend URL, Host, RequestURI and pipeline fallbacks. Prevent sharing Vary-star responses and preserve bypass bodies until delivery.
Direct dispatch bypasses later proxy filters; required filter ordering is documented and tested.
How to Verify
Shared setup
make verifystarts or reuses Minikube, the mock origin and the stack; runs the smoke and coalescing checks; and validates supported dashboard queries. It prints a report path and exits non-zero on failure. For outage evidence, runmake chaos SKIPPER_CHECKOUT=../skipper-pr-4387 NAME=origin-stale; it includes its own timed load. Requirements: Python 3.9+, Make, Bash, Git, curl, kubectl, Minikube, Docker and Go compatible with Skipper'sgo.mod. Free ports: 8080, 8081, 6379, 4318, 9091, 9999, 9911 (8089 for endless load). Dynamic URLs:.runtime/urls.txt; stop withmake stop.Validated on #4387 (
d767f636): smoke: 6,398 requests, 0 failures; origin-stale: 155 failed revalidations across 77 routes. Smoke metric checks: 42 PASS, 2 fault-only NOT EXERCISED, 0 FAIL.Focused tests
Expected: 17 focused tests pass;
make verifyconfirms coalescing (cold burst: 1 origin call; warm follow-up: 0 extra; threeVary: *requests: 3 calls). Bypass-body tests coverno-store/Vary: *, not 405/500. #4370 mishandlesVary: Accept-Languageon cold misses; #4371 fixes it. Direct origin fetches lack separate backend metrics/access-log records; ingress serve metrics/client access logging remain.cache.reval_backend_dispatchcounts refreshes only; #4387 adds dispatch-labelled origin duration.Checklist
Rollback Plan
Revert dependent PRs first, then this PR. No cache data-format migration is required.
References & Additional Context
Depends on #4369; followed by #4371 and #4372.
E2E Testing Results
E2E runs exercising dispatch under load and origin failure. Full results: larry-dalmeida/skipper-cache-e2e-tests/results
Test Scenarios:
Validation Panels:
These performance metrics demonstrate that centralized dispatch maintains efficiency and reliability across normal and degraded conditions:
Request Rate - Dispatch throughput under sustained load (bda674b smoke), ramping to ~54 req/s with zero failures. Illustrates that centralizing request preparation between cold fetches and revalidation does not introduce bottlenecks or contention. Smooth, stable throughput under high load proves the shared preparation pathway is efficient and reliable.
Origin Latency (p95) - Origin fetch latency during the same smoke run, ~9.5–10 ms p95 consistently across all response statuses (200, 304, 404, 503). Validates that preserving bypass bodies and response state does not introduce latency overhead or unnecessary re-reads. Consistent latency across different status codes confirms that centralized dispatch handles diverse request/response paths without performance degradation.
Origin Responses by Status - Response distribution at the mock origin (bda674b origin-stale scenario): 503s peak at ~25 req/s while origin fails, 200s and 304s drop accordingly, then both recover cleanly. Demonstrates proper routing and fallback behavior during origin degradation. With centralized dispatch preserving pipeline fallbacks, the system correctly handles revalidation and bypass under failure conditions—304 responses occur for revalidation, and cache-serve + bypass paths maintain isolation even when origin is unavailable.
