From d99701d6b3c6f76c2f22e517967128e28af6e81f Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Fri, 21 Aug 2026 01:19:50 +0700 Subject: [PATCH] fix(mcp): give provider-bound tool calls their own fetch budget (#10860) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Obrigado — o hop de routing (route_request) herdava o budget de 10s de management em vez do budget de 60s de upstream que web_search/web_fetch já usavam, então uma rota de 35-40s abortava só pelo lado do MCP. Validação (worktree combinado a partir de origin/release/v3.8.50, 0 conflitos): - typecheck:core limpo, complexity/cognitive-complexity dentro do baseline - tests/unit/mcp-upstream-fetch-timeout-9717.test.ts — 6/6 passando - Suíte MCP completa — 149/153 (branch) vs 143/147 (release), as 4 falhas são idênticas nos dois lados e não relacionadas (closure de package-files, resolução de bundle dist/) --- .../fixes/10860-mcp-upstream-fetch-timeout.md | 1 + docs/frameworks/MCP-SERVER.md | 2 + open-sse/mcp-server/fetchTimeout.ts | 60 ++++++ open-sse/mcp-server/server.ts | 11 +- .../mcp-upstream-fetch-timeout-9717.test.ts | 191 ++++++++++++++++++ 5 files changed, 262 insertions(+), 3 deletions(-) create mode 100644 changelog.d/fixes/10860-mcp-upstream-fetch-timeout.md create mode 100644 open-sse/mcp-server/fetchTimeout.ts create mode 100644 tests/unit/mcp-upstream-fetch-timeout-9717.test.ts diff --git a/changelog.d/fixes/10860-mcp-upstream-fetch-timeout.md b/changelog.d/fixes/10860-mcp-upstream-fetch-timeout.md new file mode 100644 index 0000000000..a23aeed2a1 --- /dev/null +++ b/changelog.d/fixes/10860-mcp-upstream-fetch-timeout.md @@ -0,0 +1 @@ +- **fix(mcp):** MCP tool calls that wait on a model provider no longer abort after 10 seconds. `omniRouteFetch` applied a single hardcoded `AbortSignal.timeout(10000)` to every internal hop, and `omniroute_route_request` — which posts to `/v1/chat/completions` and waits on the upstream provider, plus auto-combo candidate probing before a provider is even chosen — passed no signal of its own, so it inherited it. Any route slower than 10s failed from the MCP side while the identical request succeeded through the REST API. `omniroute_web_search` and `omniroute_web_fetch` in the same file already carried an explicit 60s signal, so that value is now shared by all three provider-bound calls instead of being repeated as a literal, while management reads (health, resilience, rate limits, combos, quota, usage) keep their fast-fail 10s budget so a stalled local endpoint still cannot hold a tool call open. Both budgets are overridable through `OMNIROUTE_MCP_FETCH_TIMEOUT_MS` and `OMNIROUTE_MCP_UPSTREAM_TIMEOUT_MS`, replacing the reported workaround of patching the compiled `dist/.build/next/server/chunks/*.js`; a malformed or non-positive override falls back to the default rather than disabling the timeout diff --git a/docs/frameworks/MCP-SERVER.md b/docs/frameworks/MCP-SERVER.md index 67e946528f..18877057fd 100644 --- a/docs/frameworks/MCP-SERVER.md +++ b/docs/frameworks/MCP-SERVER.md @@ -347,6 +347,8 @@ per-key path take precedence once it is. stdio has no per-caller identity (see | `OMNIROUTE_MCP_SCOPES` | (empty) | Comma-separated allowlist of scopes considered "available" by default (used when caller does not provide its own scopes) | | `OMNIROUTE_MCP_COMPRESS_DESCRIPTIONS` | (unset = on) | When set to `0/false/off/no`, disables MCP description compression at registration time | | `OMNIROUTE_MCP_DESCRIPTION_COMPRESSION` | (unset = on) | Alternate alias for the same toggle as above | +| `OMNIROUTE_MCP_FETCH_TIMEOUT_MS` | `10000` | Abort budget for internal management reads (health, resilience, combos, quota, usage) | +| `OMNIROUTE_MCP_UPSTREAM_TIMEOUT_MS` | `60000` | Abort budget for hops that wait on a provider (`route_request`, `web_search`, `web_fetch`) | | `MCP_TOOL_DENY` | (unset = no filter) | Comma-separated tool names to drop from `tools/list` (tool-cardinality reduction — see below) | | `MCP_TOOL_ALLOW` | (unset = no filter) | Comma-separated tool names to keep exclusively (allow-list mode — see below) | | `DATA_DIR` | `~/.omniroute` | Heartbeat file is written to `${DATA_DIR}/runtime/mcp-heartbeat.json` | diff --git a/open-sse/mcp-server/fetchTimeout.ts b/open-sse/mcp-server/fetchTimeout.ts new file mode 100644 index 0000000000..a9389b0c24 --- /dev/null +++ b/open-sse/mcp-server/fetchTimeout.ts @@ -0,0 +1,60 @@ +/** + * #9717 — timeout policy for the MCP server's internal server→server fetches. + * + * `omniRouteFetch` serves two call shapes with very different latency budgets: + * fast local management reads (health, resilience, combos, quota, usage) and + * calls that wait on an upstream provider. A single 10s default aborted + * `omniroute_route_request` while the upstream request was still in flight, + * even though `omniroute_web_search` / `omniroute_web_fetch` already carried + * their own explicit 60s signal in the same file for exactly that reason. + * + * Kept as a pure, dependency-free module so the policy is unit-testable without + * starting the MCP server, mirroring how `tools/poolTools.ts` keeps handlers + * separate from server wiring. + */ + +/** Local management reads — a stalled one should fail fast, not hold a tool call open. */ +export const MCP_FETCH_TIMEOUT_MS = 10_000; + +/** + * Calls that wait on an upstream provider. 60s is not a new number: it is the + * value `web_search`/`web_fetch` already used, now shared with model routing + * instead of each call site picking its own literal. + */ +export const MCP_UPSTREAM_FETCH_TIMEOUT_MS = 60_000; + +export const MCP_FETCH_TIMEOUT_ENV = "OMNIROUTE_MCP_FETCH_TIMEOUT_MS"; +export const MCP_UPSTREAM_FETCH_TIMEOUT_ENV = "OMNIROUTE_MCP_UPSTREAM_TIMEOUT_MS"; + +export type McpFetchTimeoutKind = "management" | "upstream"; + +function readPositiveIntEnv(raw: string | undefined): number | null { + if (typeof raw !== "string" || raw.trim() === "") return null; + const parsed = Number(raw); + return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : null; +} + +/** + * Resolve the timeout for one internal fetch class. An unset, malformed or + * non-positive override falls back to the built-in default rather than + * disabling the timeout — a bad env value must not turn a bounded wait into an + * unbounded one. + */ +export function resolveMcpFetchTimeoutMs( + kind: McpFetchTimeoutKind, + env: Record = process.env +): number { + const upstream = kind === "upstream"; + const override = readPositiveIntEnv( + env[upstream ? MCP_UPSTREAM_FETCH_TIMEOUT_ENV : MCP_FETCH_TIMEOUT_ENV] + ); + return override ?? (upstream ? MCP_UPSTREAM_FETCH_TIMEOUT_MS : MCP_FETCH_TIMEOUT_MS); +} + +/** `AbortSignal` for one internal fetch of the given class. */ +export function mcpFetchTimeoutSignal( + kind: McpFetchTimeoutKind, + env?: Record +): AbortSignal { + return AbortSignal.timeout(resolveMcpFetchTimeoutMs(kind, env)); +} diff --git a/open-sse/mcp-server/server.ts b/open-sse/mcp-server/server.ts index f5634816e1..b2a866e997 100644 --- a/open-sse/mcp-server/server.ts +++ b/open-sse/mcp-server/server.ts @@ -92,6 +92,7 @@ import { getDbInstance } from "../../src/lib/db/core.ts"; import { normalizeQuotaResponse } from "../../src/shared/contracts/quota.ts"; import { resolveOmniRouteBaseUrl } from "../../src/shared/utils/resolveOmniRouteBaseUrl.ts"; import { sanitizeErrorMessage } from "../utils/error.ts"; +import { mcpFetchTimeoutSignal } from "./fetchTimeout.ts"; import { getMcpModelsCatalog } from "./catalog.ts"; import { registerRadarCatalogTool } from "./radarCatalog.ts"; import type { TextToolResult } from "./toolResult.ts"; @@ -213,7 +214,7 @@ export async function omniRouteFetch(path: string, options: RequestInit = {}): P ...getInternalServiceAuthHeaders(), }; - const signal = options.signal || AbortSignal.timeout(10000); + const signal = options.signal || mcpFetchTimeoutSignal("management"); const response = await fetch(url, { ...options, headers, signal }); if (!response.ok) { @@ -518,6 +519,10 @@ async function handleRouteRequest(args: { const raw = (await omniRouteFetch("/v1/chat/completions", { method: "POST", body: JSON.stringify(body), + // #9717: this hop waits on an upstream provider (and on auto-combo + // candidate probing before one is even chosen), so it must not inherit + // the management-read budget. + signal: mcpFetchTimeoutSignal("upstream"), })) as JsonRecord; const choices = toArray(raw.choices); const firstChoice = toRecord(choices[0]); @@ -648,7 +653,7 @@ async function handleWebSearch(args: { const result = await omniRouteFetch("/v1/search", { method: "POST", body: JSON.stringify(body), - signal: AbortSignal.timeout(60000), + signal: mcpFetchTimeoutSignal("upstream"), }); await logToolCall("omniroute_web_search", args, result, Date.now() - start, true); return { content: [{ type: "text" as const, text: JSON.stringify(result, null, 2) }] }; @@ -681,7 +686,7 @@ async function handleWebFetch(args: { const result = await omniRouteFetch("/v1/web/fetch", { method: "POST", body: JSON.stringify(body), - signal: AbortSignal.timeout(60000), + signal: mcpFetchTimeoutSignal("upstream"), }); await logToolCall("omniroute_web_fetch", args, result, Date.now() - start, true); return { content: [{ type: "text" as const, text: JSON.stringify(result, null, 2) }] }; diff --git a/tests/unit/mcp-upstream-fetch-timeout-9717.test.ts b/tests/unit/mcp-upstream-fetch-timeout-9717.test.ts new file mode 100644 index 0000000000..270f56ea4f --- /dev/null +++ b/tests/unit/mcp-upstream-fetch-timeout-9717.test.ts @@ -0,0 +1,191 @@ +/** + * #9717 — the MCP server's internal fetch budget. + * + * `omniRouteFetch` applied one hardcoded 10s `AbortSignal.timeout` to every + * internal hop, including `omniroute_route_request`'s call to + * `/v1/chat/completions`. That hop waits on an upstream provider (and on + * auto-combo candidate probing before a provider is even chosen), so any route + * slower than 10s aborted from the MCP side while the same request succeeded + * through the REST API. `web_search`/`web_fetch` in the same file already used + * an explicit 60s signal, which is the value adopted here. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +const { + resolveMcpFetchTimeoutMs, + MCP_FETCH_TIMEOUT_MS, + MCP_UPSTREAM_FETCH_TIMEOUT_MS, + MCP_FETCH_TIMEOUT_ENV, + MCP_UPSTREAM_FETCH_TIMEOUT_ENV, +} = await import("../../open-sse/mcp-server/fetchTimeout.ts"); +const { createMcpServer, omniRouteFetch } = await import("../../open-sse/mcp-server/server.ts"); + +type RegisteredTool = { + handler: ( + args: unknown, + extra?: unknown + ) => Promise<{ content?: Array<{ type: string; text: string }>; isError?: boolean }>; +}; + +function getRegisteredHandler(server: unknown, toolName: string) { + const registry = (server as { _registeredTools?: Record }) + ._registeredTools; + assert.ok(registry, "McpServer should expose _registeredTools"); + const tool = registry[toolName]; + assert.ok(tool, `${toolName} must be registered on the live MCP server`); + return tool.handler; +} + +const CHAT_COMPLETION_BODY = { + choices: [{ message: { content: "ok" } }], + model: "test-model", + usage: { prompt_tokens: 1, completion_tokens: 1 }, + provider: "test-provider", +}; + +/** + * Stand-in for `fetch` that answers after `delayMs` but honours an abort signal + * the same way the real implementation does — a stub that ignored the signal + * would make every timeout assertion below pass vacuously. + */ +function stubFetch(delayMs: number, seen: { signals: AbortSignal[] }) { + const original = globalThis.fetch; + globalThis.fetch = ((_url: unknown, init?: { signal?: AbortSignal }) => { + const signal = init?.signal; + if (signal) seen.signals.push(signal); + return new Promise((resolve, reject) => { + const timer = setTimeout( + () => + resolve({ + ok: true, + status: 200, + json: async () => CHAT_COMPLETION_BODY, + text: async () => JSON.stringify(CHAT_COMPLETION_BODY), + }), + delayMs + ); + const abort = () => { + clearTimeout(timer); + reject(signal?.reason ?? new Error("aborted")); + }; + if (signal?.aborted) return abort(); + signal?.addEventListener("abort", abort, { once: true }); + }); + }) as typeof globalThis.fetch; + return () => { + globalThis.fetch = original; + }; +} + +function withEnv(vars: Record) { + const previous = new Map(); + for (const [key, value] of Object.entries(vars)) { + previous.set(key, process.env[key]); + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + return () => { + for (const [key, value] of previous) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + }; +} + +async function callRouteRequest() { + const handler = getRegisteredHandler(createMcpServer(), "omniroute_route_request"); + return handler( + { model: "test-model", messages: [{ role: "user", content: "hi" }] }, + { authInfo: { clientId: "test-9717", scopes: ["execute:completions"] } } + ); +} + +// ── Policy ──────────────────────────────────────────────────────────────── + +test("#9717: the upstream budget is larger than the management budget", () => { + assert.equal(resolveMcpFetchTimeoutMs("management"), MCP_FETCH_TIMEOUT_MS); + assert.equal(resolveMcpFetchTimeoutMs("upstream"), MCP_UPSTREAM_FETCH_TIMEOUT_MS); + assert.equal(MCP_FETCH_TIMEOUT_MS, 10_000); + assert.equal( + MCP_UPSTREAM_FETCH_TIMEOUT_MS, + 60_000, + "matches the signal web_search/web_fetch already used" + ); + assert.ok(MCP_UPSTREAM_FETCH_TIMEOUT_MS > MCP_FETCH_TIMEOUT_MS); +}); + +test("#9717: each budget reads its own env override", () => { + const env = { + [MCP_FETCH_TIMEOUT_ENV]: "1234", + [MCP_UPSTREAM_FETCH_TIMEOUT_ENV]: "222222", + }; + assert.equal(resolveMcpFetchTimeoutMs("management", env), 1234); + assert.equal(resolveMcpFetchTimeoutMs("upstream", env), 222222); +}); + +test("#9717: a malformed override falls back to the default instead of disabling the timeout", () => { + for (const bad of ["", " ", "0", "-1", "abc", "60000.5", "NaN", "Infinity"]) { + assert.equal( + resolveMcpFetchTimeoutMs("upstream", { [MCP_UPSTREAM_FETCH_TIMEOUT_ENV]: bad }), + MCP_UPSTREAM_FETCH_TIMEOUT_MS, + `"${bad}" must not become the effective timeout` + ); + } +}); + +// ── Wiring ──────────────────────────────────────────────────────────────── + +test("#9717: route_request is bound to the upstream budget, not the management default", async () => { + const seen = { signals: [] as AbortSignal[] }; + const restoreEnv = withEnv({ [MCP_UPSTREAM_FETCH_TIMEOUT_ENV]: "40" }); + const restoreFetch = stubFetch(400, seen); + try { + const result = await callRouteRequest(); + assert.equal( + result.isError, + true, + "with the upstream budget set to 40ms a 400ms upstream must abort — before #9717 this " + + "call ignored that setting and used the hardcoded 10s default, so it returned a result" + ); + assert.ok(seen.signals.length > 0, "the routing hop must carry an abort signal"); + } finally { + restoreFetch(); + restoreEnv(); + } +}); + +test("#9717: route_request outlives the management budget", async () => { + const seen = { signals: [] as AbortSignal[] }; + const restoreEnv = withEnv({ + [MCP_FETCH_TIMEOUT_ENV]: "40", + [MCP_UPSTREAM_FETCH_TIMEOUT_ENV]: undefined, + }); + const restoreFetch = stubFetch(400, seen); + try { + const result = await callRouteRequest(); + assert.notEqual( + result.isError, + true, + "a 400ms upstream must survive: the routing hop must not inherit the 40ms management budget" + ); + } finally { + restoreFetch(); + restoreEnv(); + } +}); + +test("#9717: management reads honour their own override", async () => { + const seen = { signals: [] as AbortSignal[] }; + const restoreEnv = withEnv({ [MCP_FETCH_TIMEOUT_ENV]: "40" }); + const restoreFetch = stubFetch(400, seen); + try { + await assert.rejects( + () => omniRouteFetch("/api/monitoring/health"), + "a management read must abort at its configured budget" + ); + } finally { + restoreFetch(); + restoreEnv(); + } +});