fix: use BUFFERED request body mode for reliable ClearRouteCache#1201
Open
maleck13 wants to merge 1 commit into
Open
fix: use BUFFERED request body mode for reliable ClearRouteCache#1201maleck13 wants to merge 1 commit into
maleck13 wants to merge 1 commit into
Conversation
STREAMED body mode causes Envoy to silently ignore header mutations and ClearRouteCache in the ext_proc body phase. This led to intermittent (~15-25%) routing failures where hairpin initialize requests were handled by the broker instead of the backend MCP server. BUFFERED mode guarantees the route cache clear takes effect before Envoy selects an upstream. Limited to 1MB request body (Envoy default) which is sufficient for MCP JSON-RPC payloads. Also adds x-client-id header to broker and lazyinit requests, and improves server2 test server logging. Signed-off-by: craig <cbrookes@redhat.com>
📝 WalkthroughWalkthroughThe PR switches Envoy ext_proc request-body handling to BUFFERED in generated and checked-in config, adds ChangesBuffered ext_proc routing and client headers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
high-risk
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/clients/clients.go`:
- Line 97: The assignment to passThroughHeaders in Initialize can panic when the
map argument is nil. Add a defensive nil guard before setting the x-client-id
entry, and only write the lazyinit value when passThroughHeaders is non-nil; use
the Initialize function and the passThroughHeaders map name to locate the
change.
In `@internal/tests/server2/server2.go`:
- Around line 225-253: responseRecorder is missing http.Flusher forwarding,
which breaks StreamableHTTPServer SSE streaming when logResponse wraps the
handler. Update responseRecorder to preserve streaming behavior by forwarding
Flush to the underlying ResponseWriter when it implements http.Flusher, and
ensure logResponse still uses responseRecorder without interfering with
streaming responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 006be1ed-f7bd-4cd9-840b-00ffa976e6e9
📒 Files selected for processing (7)
config/istio/envoyfilter.yamldocs/design/routing.mdinternal/broker/upstream/mcp.gointernal/clients/clients.gointernal/controller/mcpgatewayextension_controller.gointernal/mcp-router/response_handlers.gointernal/tests/server2/server2.go
💤 Files with no reviewable changes (1)
- internal/mcp-router/response_handlers.go
|
|
||
| url := buildHairpinURL(gatewayHost, mcpPath) | ||
| hairpinHTTPClient := hairpinClientPool.Get(conf.Hostname) | ||
| passThroughHeaders["x-client-id"] = "lazyinit" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# find call sites of clients.Initialize and inspect the map arg
rg -nP -C4 '\bInitialize\s*\(' --type=go -g '!**/*_test.go'Repository: Kuadrant/mcp-gateway
Length of output: 158
Defensive guard for passThroughHeaders assignment
The write at line 97 passThroughHeaders["x-client-id"] = "lazyinit" will panic if the argument is nil. While the current caller in request_handlers.go initializes the map, Initialize is exported and any future caller passing nil will crash the service.
Add a nil check to prevent runtime panics:
Defensive guard
url := buildHairpinURL(gatewayHost, mcpPath)
hairpinHTTPClient := hairpinClientPool.Get(conf.Hostname)
+ if passThroughHeaders == nil {
+ passThroughHeaders = map[string]string{}
+ }
passThroughHeaders["x-client-id"] = "lazyinit"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| passThroughHeaders["x-client-id"] = "lazyinit" | |
| if passThroughHeaders == nil { | |
| passThroughHeaders = map[string]string{} | |
| } | |
| passThroughHeaders["x-client-id"] = "lazyinit" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/clients/clients.go` at line 97, The assignment to passThroughHeaders
in Initialize can panic when the map argument is nil. Add a defensive nil guard
before setting the x-client-id entry, and only write the lazyinit value when
passThroughHeaders is non-nil; use the Initialize function and the
passThroughHeaders map name to locate the change.
Comment on lines
+225
to
+253
| type responseRecorder struct { | ||
| http.ResponseWriter | ||
| status int | ||
| } | ||
|
|
||
| func (r *responseRecorder) WriteHeader(code int) { | ||
| r.status = code | ||
| r.ResponseWriter.WriteHeader(code) | ||
| } | ||
|
|
||
| func logResponse(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| rec := &responseRecorder{ResponseWriter: w, status: http.StatusOK} | ||
| next.ServeHTTP(rec, r) | ||
| reqSession := r.Header.Get("Mcp-Session-Id") | ||
| if reqSession == "" { | ||
| reqSession = "-" | ||
| } | ||
| respSession := rec.Header().Get("Mcp-Session-Id") | ||
| if respSession == "" { | ||
| respSession = "-" | ||
| } | ||
| clientID := r.Header.Get("X-Client-Id") | ||
| if clientID == "" { | ||
| clientID = "-" | ||
| } | ||
| log.Printf("%s %s %d req-session=%s resp-session=%s x-client-id=%s", r.Method, r.URL.Path, rec.status, reqSession, respSession, clientID) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check mcp-go streamable server for Flusher usage
fd -t f -e go . | xargs rg -nP 'http\.Flusher|\.Flush\(\)' -g '*streamable*' 2>/dev/null
rg -nP 'http\.Flusher|\.Flush\(\)' $(go env GOMODCACHE 2>/dev/null)/github.com/mark3labs/mcp-go* 2>/dev/null | headRepository: Kuadrant/mcp-gateway
Length of output: 158
responseRecorder breaks streaming by missing http.Flusher support
responseRecorder embeds http.ResponseWriter but lacks a Flush() method. The underlying StreamableHTTPServer requires http.Flusher for SSE responses; wrapping it causes streaming calls to stall or fail.
Fix: Forward `Flush`
func (r *responseRecorder) WriteHeader(code int) {
r.status =code
r.ResponseWriter.WriteHeader(code)
+}
+
+func (r *responseRecorder) Flush() {
+ if f, ok := r.ResponseWriter.(http.Flusher); ok {
+ f.Flush()
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type responseRecorder struct { | |
| http.ResponseWriter | |
| status int | |
| } | |
| func (r *responseRecorder) WriteHeader(code int) { | |
| r.status = code | |
| r.ResponseWriter.WriteHeader(code) | |
| } | |
| func logResponse(next http.Handler) http.Handler { | |
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| rec := &responseRecorder{ResponseWriter: w, status: http.StatusOK} | |
| next.ServeHTTP(rec, r) | |
| reqSession := r.Header.Get("Mcp-Session-Id") | |
| if reqSession == "" { | |
| reqSession = "-" | |
| } | |
| respSession := rec.Header().Get("Mcp-Session-Id") | |
| if respSession == "" { | |
| respSession = "-" | |
| } | |
| clientID := r.Header.Get("X-Client-Id") | |
| if clientID == "" { | |
| clientID = "-" | |
| } | |
| log.Printf("%s %s %d req-session=%s resp-session=%s x-client-id=%s", r.Method, r.URL.Path, rec.status, reqSession, respSession, clientID) | |
| }) | |
| } | |
| type responseRecorder struct { | |
| http.ResponseWriter | |
| status int | |
| } | |
| func (r *responseRecorder) WriteHeader(code int) { | |
| r.status = code | |
| r.ResponseWriter.WriteHeader(code) | |
| } | |
| func (r *responseRecorder) Flush() { | |
| if f, ok := r.ResponseWriter.(http.Flusher); ok { | |
| f.Flush() | |
| } | |
| } | |
| func logResponse(next http.Handler) http.Handler { | |
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| rec := &responseRecorder{ResponseWriter: w, status: http.StatusOK} | |
| next.ServeHTTP(rec, r) | |
| reqSession := r.Header.Get("Mcp-Session-Id") | |
| if reqSession == "" { | |
| reqSession = "-" | |
| } | |
| respSession := rec.Header().Get("Mcp-Session-Id") | |
| if respSession == "" { | |
| respSession = "-" | |
| } | |
| clientID := r.Header.Get("X-Client-Id") | |
| if clientID == "" { | |
| clientID = "-" | |
| } | |
| log.Printf("%s %s %d req-session=%s resp-session=%s x-client-id=%s", r.Method, r.URL.Path, rec.status, reqSession, respSession, clientID) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tests/server2/server2.go` around lines 225 - 253, responseRecorder
is missing http.Flusher forwarding, which breaks StreamableHTTPServer SSE
streaming when logResponse wraps the handler. Update responseRecorder to
preserve streaming behavior by forwarding Flush to the underlying ResponseWriter
when it implements http.Flusher, and ensure logResponse still uses
responseRecorder without interfering with streaming responses.
jasonmadigan
approved these changes
Jun 26, 2026
jasonmadigan
left a comment
jasonmadigan
left a comment
Member
There was a problem hiding this comment.
lgtm. the docs CI failure looks like something that was broken on main already - found because router.md was touched
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
2 participants
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.
STREAMED body mode causes Envoy to silently ignore header mutations and ClearRouteCache in the ext_proc body phase. This led to intermittent (~15-25%) routing failures where hairpin initialize requests were handled by the broker instead of the backend MCP server.
BUFFERED mode guarantees the route cache clear takes effect before Envoy selects an upstream. Limited to 1MB request body (Envoy default) which is sufficient for MCP JSON-RPC payloads.
Also adds x-client-id header to broker and lazyinit requests, and improves server2 test server logging.
Summary by CodeRabbit
Bug Fixes
tools/callrouting failures.Documentation
Chores