mirror of
https://github.com/lukaszraczylo/traefik-llmgateway.git
synced 2026-10-20 05:56:56 +00:00
fix(mcp): backend timeouts, response validity, session handshake fallback
Review round 2 fix batch for the federated /mcp endpoint, driven by a
live probe against all 11 production MCP servers behind this gateway
through the byte-pipe proxy (session-less tools/list): 8 answer bare,
readitall and fetch reject it and need a session handshake, playwright
is a legacy HTTP+SSE-transport server (400 Missing sessionId) that no
handshake fixes.
MF1: every backend call now runs under its own context.WithTimeout
derived from r.Context() — toolsListBackendTimeout (20s) per fan-out
call, toolsCallBackendTimeout (120s) for tools/call, named constants.
MF2: doBackendJSONRPC rejects any non-2xx upstream status as an error
(previously unchecked — a non-JSON-RPC error body only failed by
accident of not parsing). parseBackendJSONRPC additionally rejects a
validly-JSON body that carries neither "result" nor "error" — the
exact silent-failure shape that would otherwise decode into an
all-zero jsonrpcResponse and hand the client a bare
{"jsonrpc":"2.0","id":1}. mcpFederatedToolsCall carries a
defensive final guard against the same shape before relaying.
MF3: mcpBackendCall is now bare-first-with-fallback — attempt the
bare call; on a JSON-RPC error OR HTTP 4xx (never a network failure,
timeout, or 5xx), retry once via initialize -> capture Mcp-Session-Id
-> retry with the header -> best-effort DELETE close. A legacy
HTTP+SSE-transport server (URL path ending "/sse") is excluded from
federation entirely before ever reaching this — isLegacySSETransportURL,
folded into allowedMCPServerNames — since no handshake fixes a
different transport; it stays reachable via the per-server proxy. When
every attempted server fails, tools/list returns a JSON-RPC internal
error ("no MCP server reachable") instead of a silently-empty list.
SF6: defaultMCPProtocolVersion -> "2025-11-25"; a caller's own
protocolVersion is echoed back only from a small explicit allowlist
(knownMCPProtocolVersions), falling back to the default otherwise —
never echoes an arbitrary caller string as this gateway's own claimed
capability.
SF8: GET/DELETE/etc. on exactly /mcp is now 405 with Allow: POST,
checked before auth (llmgateway.go).
Folded minors: countTargetRequests batches per-target counter writes
for tools/list's whole fan-out into ONE storeIncrMulti call after
wg.Wait() (was one call per server); every "actually contacted" doc
comment now says "attempted" (attribution is attempt-, not
success-, accounting); lastSSEDataLine documents its no-cross-message-
id-correlation limitation; routes_unified.go's stale buildLimitScopes/
withTotalScope comments (which claimed handleAdminAPI calls
buildLimitScopes directly — it never has) are corrected; jsonrpcRequest
documents why its "jsonrpc" field is decoded but never validated.
Tests: ~30 new/updated cases covering per-backend timeout derivation
(via the shorter-of-two-context-deadlines technique, no real 20s/120s
waits), non-2xx rejection, the empty-envelope guard end-to-end,
handshake-trigger classification (table-driven: JSON-RPC error and
HTTP 4xx trigger, HTTP 500 and network failure do not), the full
handshake sequence (bare -> initialize -> retry -> close, asserted
call-by-call), handshake-gives-up-too, all-servers-failed, legacy-SSE
exclusion (never contacted, in both tools/list and tools/call),
protocol-version allowlist (all 4 versions + one rejected), 405/Allow,
and best-effort session-close failure never surfacing to the caller.
This commit is contained in:
+19
-10
@@ -244,10 +244,15 @@ func (g *Gateway) runUnified(w http.ResponseWriter, r *http.Request, u *user, gr
|
||||
// contract (admin.go) that the dashboard's usage table already promised.
|
||||
//
|
||||
// Callers metering actual LLM traffic wrap this result in withTotalScope
|
||||
// before passing it to checkAndCount/account; handleAdminAPI (admin.go)
|
||||
// calls this directly, without withTotalScope, so an admin request's own
|
||||
// req/min-req/day accounting never contributes to the total-scope
|
||||
// LLM-traffic series (v0.2 data-layer task).
|
||||
// before passing it to checkAndCount/account. handleAdminAPI (admin.go)
|
||||
// never calls buildLimitScopes at all — its own buildAdminUsage builds
|
||||
// {kind, id, limits} literals directly from authStore.snapshot's user/group
|
||||
// listing, for currentUsage's read-only purposes — and, separately, never
|
||||
// calls checkAndCount either (handleAdminAPI's own doc comment: admin
|
||||
// traffic must never move req/min-req/day statistics). Fixed a stale
|
||||
// version of this comment (review round 2, 2026-08-21) that claimed
|
||||
// handleAdminAPI "calls this directly, without withTotalScope" — it never
|
||||
// called this function at all, on any version of this file.
|
||||
func buildLimitScopes(u *user, grp *group) []limitScope {
|
||||
return []limitScope{
|
||||
{limits: u.limits, kind: "user", id: u.name},
|
||||
@@ -259,12 +264,16 @@ func buildLimitScopes(u *user, grp *group) []limitScope {
|
||||
// (totalScopeKind/totalScopeID, limits.go) appended — the single helper
|
||||
// every metered route (runUnified above; resolveMediaRequest,
|
||||
// routes_media.go; handlePassthrough, routes_passthrough.go;
|
||||
// handleTargetProxy, mcp_a2a.go) calls around its own buildLimitScopes
|
||||
// result, so none of them can forget it and none of them duplicate the
|
||||
// scope literal. Deliberately not folded into buildLimitScopes itself:
|
||||
// that helper is also called directly by handleAdminAPI (admin.go) for
|
||||
// its own req/min-req/day accounting, and admin traffic must never
|
||||
// contribute to the total LLM-traffic scope.
|
||||
// handleTargetProxy, mcp_a2a.go; handleMCPFederated, mcp_federation.go)
|
||||
// calls around its own buildLimitScopes result, so none of them can forget
|
||||
// it and none of them duplicate the scope literal. Deliberately not folded
|
||||
// into buildLimitScopes itself: buildLimitScopes' own doc comment now
|
||||
// explains why admin traffic needs no special-casing here at all —
|
||||
// handleAdminAPI never calls either function, so there was never a real
|
||||
// "admin traffic must not contribute to total" case for this split to
|
||||
// guard against; the split is kept anyway because buildLimitScopes'
|
||||
// {user, group} pair and the synthetic total scope are conceptually
|
||||
// different additions, worth two names.
|
||||
func withTotalScope(scopes []limitScope) []limitScope {
|
||||
return append(scopes, limitScope{kind: totalScopeKind, id: totalScopeID, limits: nil})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user