diff --git a/docs/reliability.md b/docs/reliability.md new file mode 100644 index 0000000..22e8b49 --- /dev/null +++ b/docs/reliability.md @@ -0,0 +1,37 @@ +# Reliability: don't let a bad LLM take mcpctl down + +The homelab model changes often (fast ↔ thinking, model swaps, backends that +drift or go down). mcpctl must stay responsive and honest through all of it. + +## Principle + +**LLM-*optional* operations must be time-bounded, fall back deterministically, +and report the degradation — never hang and never degrade silently.** + +- **Bounded:** every optional LLM call is wrapped in + [`withTimeout`](../src/mcplocal/src/util/with-timeout.ts) (Promise.race + an + `AbortSignal` so fetch-based providers actually cancel). A thinking model that + streams for minutes can never block the caller. +- **Deterministic fallback:** when the LLM times out or errors, use the + non-LLM path (priority/keyword ordering, byte-range pages). +- **Loud, not silent:** log the reason (`[gate] …`, `[pagination] …`) and tell + the user. `begin_session` prepends `⚠ Smart prompt-selection unavailable + ()…` and sets `degraded: true` + `degradedReason` on the audit + `gate_decision` event. + +Applied in: the gate's `begin_session` prompt selection +(`proxymodel/plugins/gate.ts`, cap `MCPCTL_GATE_LLM_TIMEOUT_MS`, default 8s) and +pagination's smart index (`llm/pagination.ts`, `MCPCTL_PAGINATION_LLM_TIMEOUT_MS`, +default 10s). `read_prompts` is LLM-free by design. + +Note: the gate's prompt-ranking uses the **heavy client provider's own model** — +it deliberately does *not* force the project's vLLM model onto it (doing so made +every selection fail silently when the model wasn't anthropic-servable). + +## LLM-*essential* operations + +Chat needs the LLM — it can't fall back. It must **fail fast with a clear, +actionable message** instead of a vague one. Chat surfaces the upstream status + +body (which names the model + reason), e.g. +`LLM returned no completion (HTTP 400): {"error":"model deepseek-v4-flash not found"}`, +rather than "Adapter returned no choice". diff --git a/src/mcpd/src/services/chat.service.ts b/src/mcpd/src/services/chat.service.ts index 65556d0..16d8d07 100644 --- a/src/mcpd/src/services/chat.service.ts +++ b/src/mcpd/src/services/chat.service.ts @@ -254,7 +254,11 @@ export class ChatService { const result = await this.runOneInference(ctx); const choice = extractChoice(result.body); if (choice === null) { - throw new Error(`Adapter returned no choice (status ${String(result.status)})`); + // Surface the upstream body (LiteLLM/vLLM names the model + reason, + // e.g. "model deepseek-v4-flash not found") instead of a vague + // "no choice" — the caller needs to know WHICH model failed and why. + const detail = typeof result.body === 'string' ? result.body : JSON.stringify(result.body); + throw new Error(`LLM returned no completion (HTTP ${String(result.status)}): ${detail.slice(0, 400)}`); } if (choice.tool_calls !== undefined && choice.tool_calls.length > 0) { // Tool turns: keep `content` literal — even if empty — because the diff --git a/src/mcplocal/src/gate/llm-selector.ts b/src/mcplocal/src/gate/llm-selector.ts index 016b211..46f0d99 100644 --- a/src/mcplocal/src/gate/llm-selector.ts +++ b/src/mcplocal/src/gate/llm-selector.ts @@ -23,13 +23,13 @@ export interface LlmSelectionResult { export class LlmPromptSelector { constructor( private readonly providerRegistry: ProviderRegistry, - private readonly modelOverride?: string, ) {} async selectPrompts( tags: string[], promptIndex: PromptIndexForLlm[], getSystemPromptFn?: SystemPromptFetcher, + signal?: AbortSignal, ): Promise { const DEFAULT_SYSTEM_PROMPT = `You are a context selection assistant. Given a developer's task keywords and a list of available project prompts, select which prompts are relevant to their work. Return a JSON object with "selectedNames" (array of prompt names) and "reasoning" (brief explanation). Priority 10 prompts must always be included.`; const systemPrompt = getSystemPromptFn @@ -48,6 +48,9 @@ Select the relevant prompts. Return JSON: { "selectedNames": [...], "reasoning": throw new Error('No heavy LLM provider available'); } + // Deliberately NOT setting `model` here. Prompt-ranking only needs a + // working LLM; forcing the project's vLLM model onto the (anthropic) heavy + // provider made every selection fail silently. Use the provider's own model. const completionOptions: import('../providers/types.js').CompletionOptions = { messages: [ { role: 'system', content: systemPrompt }, @@ -55,10 +58,8 @@ Select the relevant prompts. Return JSON: { "selectedNames": [...], "reasoning": ], temperature: 0, maxTokens: 1024, + ...(signal ? { signal } : {}), }; - if (this.modelOverride) { - completionOptions.model = this.modelOverride; - } const result = await provider.complete(completionOptions); diff --git a/src/mcplocal/src/llm/pagination.ts b/src/mcplocal/src/llm/pagination.ts index 33c2b81..cfb7e52 100644 --- a/src/mcplocal/src/llm/pagination.ts +++ b/src/mcplocal/src/llm/pagination.ts @@ -2,6 +2,10 @@ import { randomUUID } from 'node:crypto'; import type { ProviderRegistry } from '../providers/registry.js'; import { estimateTokens } from './token-counter.js'; import type { SystemPromptFetcher } from '../proxymodel/types.js'; +import { withTimeout } from '../util/with-timeout.js'; + +/** Cap on the LLM smart-index call; on timeout, fall back to byte-range pages. */ +const PAGINATION_LLM_TIMEOUT_MS = Number(process.env['MCPCTL_PAGINATION_LLM_TIMEOUT_MS'] ?? '10000'); // --- Configuration --- @@ -260,15 +264,22 @@ export class ResponsePaginator { ? await this.getSystemPrompt('llm-pagination-index', PAGINATION_INDEX_SYSTEM_PROMPT) : PAGINATION_INDEX_SYSTEM_PROMPT; - const result = await provider.complete({ - messages: [ - { role: 'system', content: systemPrompt }, - { role: 'user', content: `Tool: ${toolName}\nTotal size: ${String(raw.length)} chars, ${String(pages.length)} pages\n\n${previews}` }, - ], - maxTokens: this.config.indexMaxTokens, - temperature: 0, - ...(this.modelOverride ? { model: this.modelOverride } : {}), - }); + // Time-bounded: a slow LLM must not stall pagination — the caller's catch + // falls back to the simple byte-range index (visibly, via console.error). + const result = await withTimeout( + (signal) => provider.complete({ + messages: [ + { role: 'system', content: systemPrompt }, + { role: 'user', content: `Tool: ${toolName}\nTotal size: ${String(raw.length)} chars, ${String(pages.length)} pages\n\n${previews}` }, + ], + maxTokens: this.config.indexMaxTokens, + temperature: 0, + signal, + ...(this.modelOverride ? { model: this.modelOverride } : {}), + }), + PAGINATION_LLM_TIMEOUT_MS, + 'pagination smart-index', + ); // LLMs often wrap JSON in ```json ... ``` fences — strip them const cleaned = result.content.replace(/^```(?:json)?\s*\n?/i, '').replace(/\n?```\s*$/i, '').trim(); diff --git a/src/mcplocal/src/providers/anthropic.ts b/src/mcplocal/src/providers/anthropic.ts index de3a855..84f5db9 100644 --- a/src/mcplocal/src/providers/anthropic.ts +++ b/src/mcplocal/src/providers/anthropic.ts @@ -45,7 +45,7 @@ export class AnthropicProvider implements LlmProvider { })); } - const response = await this.request(body); + const response = await this.request(body, options.signal); return parseAnthropicResponse(response); } @@ -72,8 +72,12 @@ export class AnthropicProvider implements LlmProvider { } } - private request(body: unknown): Promise { + private request(body: unknown, signal?: AbortSignal): Promise { return new Promise((resolve, reject) => { + if (signal?.aborted === true) { + reject(new Error('request aborted')); + return; + } const payload = JSON.stringify(body); const isOAuth = this.apiKey.startsWith('sk-ant-oat'); const opts = { @@ -109,6 +113,10 @@ export class AnthropicProvider implements LlmProvider { req.destroy(); reject(new Error('Request timed out')); }); + signal?.addEventListener('abort', () => { + req.destroy(); + reject(new Error('request aborted')); + }, { once: true }); req.write(payload); req.end(); }); diff --git a/src/mcplocal/src/providers/openai.ts b/src/mcplocal/src/providers/openai.ts index 1adf0b6..6adf330 100644 --- a/src/mcplocal/src/providers/openai.ts +++ b/src/mcplocal/src/providers/openai.ts @@ -56,7 +56,7 @@ export class OpenAiProvider implements LlmProvider { })); } - const response = await this.request('/v1/chat/completions', body); + const response = await this.request('/v1/chat/completions', body, 'POST', options.signal); return parseResponse(response); } @@ -75,8 +75,12 @@ export class OpenAiProvider implements LlmProvider { } } - private request(path: string, body: unknown, method = 'POST'): Promise { + private request(path: string, body: unknown, method = 'POST', signal?: AbortSignal): Promise { return new Promise((resolve, reject) => { + if (signal?.aborted === true) { + reject(new Error('request aborted')); + return; + } const url = new URL(path, this.baseUrl); const isHttps = url.protocol === 'https:'; const transport = isHttps ? https : http; @@ -112,6 +116,10 @@ export class OpenAiProvider implements LlmProvider { req.destroy(); reject(new Error('Request timed out')); }); + signal?.addEventListener('abort', () => { + req.destroy(); + reject(new Error('request aborted')); + }, { once: true }); if (payload) req.write(payload); req.end(); }); diff --git a/src/mcplocal/src/providers/types.ts b/src/mcplocal/src/providers/types.ts index ae867e4..479b2aa 100644 --- a/src/mcplocal/src/providers/types.ts +++ b/src/mcplocal/src/providers/types.ts @@ -42,6 +42,8 @@ export interface CompletionOptions { temperature?: number; maxTokens?: number; model?: string; + /** Cancel the in-flight request (fetch-based providers forward it to fetch). */ + signal?: AbortSignal; } /** LLM provider tier. 'fast' = local inference, 'heavy' = cloud reasoning. */ diff --git a/src/mcplocal/src/proxymodel/plugins/gate.ts b/src/mcplocal/src/proxymodel/plugins/gate.ts index e1987d1..24f057d 100644 --- a/src/mcplocal/src/proxymodel/plugins/gate.ts +++ b/src/mcplocal/src/proxymodel/plugins/gate.ts @@ -15,6 +15,11 @@ import { TagMatcher, extractKeywordsFromToolCall, tokenizeDescription } from '.. import type { TagMatchResult } from '../../gate/tag-matcher.js'; import { LlmPromptSelector } from '../../gate/llm-selector.js'; import type { ProviderRegistry } from '../../providers/registry.js'; +import { withTimeout, TimeoutError } from '../../util/with-timeout.js'; + +/** Cap on the gate's LLM prompt-selection. A slow/thinking LLM must never block + * begin_session — on timeout we fall back to deterministic tag matching. */ +const GATE_LLM_TIMEOUT_MS = Number(process.env['MCPCTL_GATE_LLM_TIMEOUT_MS'] ?? '8000'); export interface GatePluginConfig { gated?: boolean; @@ -29,7 +34,7 @@ export function createGatePlugin(config: GatePluginConfig = {}): ProxyModelPlugi const isGated = config.gated !== false; const tagMatcher = new TagMatcher(config.byteBudget); const llmSelector = config.providerRegistry - ? new LlmPromptSelector(config.providerRegistry, config.modelOverride) + ? new LlmPromptSelector(config.providerRegistry) : null; // Per-session state tracking (plugin-scoped, not global SessionGate) @@ -266,9 +271,12 @@ async function handleBeginSession( const promptIndex = await ctx.fetchPromptIndex(); - // Primary: LLM selection. Fallback: deterministic tag matching. + // Primary: LLM selection (time-bounded). Fallback: deterministic tag matching. + // A slow/thinking/down LLM must never block begin_session, and if we fall + // back the user is told (degradedReason) instead of silently degrading. let matchResult: TagMatchResult; let reasoning = ''; + let degradedReason: string | null = null; if (llmSelector) { try { @@ -279,7 +287,11 @@ async function handleBeginSession( chapters: p.chapters, })); const getSystemPromptFn = ctx.getSystemPrompt.bind(ctx); - const llmResult = await llmSelector.selectPrompts(tags, llmIndex, getSystemPromptFn); + const llmResult = await withTimeout( + (signal) => llmSelector.selectPrompts(tags, llmIndex, getSystemPromptFn, signal), + GATE_LLM_TIMEOUT_MS, + 'gate LLM prompt-selection', + ); reasoning = llmResult.reasoning; const selectedSet = new Set(llmResult.selectedNames); @@ -291,7 +303,12 @@ async function handleBeginSession( selected, ); matchResult.remaining = [...matchResult.remaining, ...remaining]; - } catch { + } catch (err) { + degradedReason = err instanceof TimeoutError + ? `LLM prompt-selection timed out after ${String(GATE_LLM_TIMEOUT_MS)}ms` + : `LLM prompt-selection failed: ${(err as Error).message}`; + // Loud, not silent — visible in mcplocal logs. + console.error(`[gate] ${degradedReason} — falling back to priority-ordered prompts`); matchResult = tagMatcher.match(tags, promptIndex); } } else { @@ -313,12 +330,20 @@ async function handleBeginSession( clientIntent: { tags, description: description ?? null }, matchedPrompts: matchResult.fullContent.map((p) => p.name), reasoning: reasoning || null, + degraded: degradedReason !== null, + degradedReason, }, }); // Build response const responseParts: string[] = []; + if (degradedReason !== null) { + responseParts.push( + `⚠ Smart prompt-selection unavailable (${degradedReason}). ` + + `Showing priority-ordered prompts — still relevant, just not LLM-ranked.\n`, + ); + } if (reasoning) { responseParts.push(`Selection reasoning: ${reasoning}\n`); } diff --git a/src/mcplocal/src/util/with-timeout.ts b/src/mcplocal/src/util/with-timeout.ts new file mode 100644 index 0000000..c67c720 --- /dev/null +++ b/src/mcplocal/src/util/with-timeout.ts @@ -0,0 +1,40 @@ +/** + * Bound an async operation with a timeout. + * + * Reliability principle: any LLM-*optional* operation (gate prompt-selection, + * pagination summaries) must be time-bounded so a slow/hung LLM (e.g. a + * thinking model that streams for minutes) can never block the caller. On + * timeout the returned promise rejects with a {@link TimeoutError}; callers are + * expected to fall back deterministically and report the degradation. + * + * The passed `AbortSignal` is aborted on timeout so fetch-based providers can + * cancel the in-flight request. Providers that ignore the signal are still + * unblocked immediately by the race (their orphaned request settles harmlessly + * in the background). + */ +export class TimeoutError extends Error { + constructor(label: string, ms: number) { + super(`${label} timed out after ${ms}ms`); + this.name = 'TimeoutError'; + } +} + +export async function withTimeout( + run: (signal: AbortSignal) => Promise, + ms: number, + label: string, +): Promise { + const controller = new AbortController(); + let timer: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timer = setTimeout(() => { + controller.abort(); + reject(new TimeoutError(label, ms)); + }, ms); + }); + try { + return await Promise.race([run(controller.signal), timeout]); + } finally { + if (timer !== undefined) clearTimeout(timer); + } +} diff --git a/src/mcplocal/tests/llm-selector.test.ts b/src/mcplocal/tests/llm-selector.test.ts index 3012465..91aa890 100644 --- a/src/mcplocal/tests/llm-selector.test.ts +++ b/src/mcplocal/tests/llm-selector.test.ts @@ -89,17 +89,30 @@ describe('LlmPromptSelector', () => { expect(call.temperature).toBe(0); }); - it('passes model override to complete options', async () => { - const provider = makeMockProvider( - '{ "selectedNames": [], "reasoning": "" }', - ); + it('does not force a model override — prompt-ranking uses the heavy provider\'s own model', async () => { + // Regression: forcing the project's vLLM model (e.g. deepseek-v4-*) onto the + // anthropic heavy provider made every selection fail silently. The selector + // must leave `model` unset so it uses whatever the heavy provider serves. + const provider = makeMockProvider('{ "selectedNames": [], "reasoning": "" }'); const registry = makeRegistry(provider); - const selector = new LlmPromptSelector(registry, 'gemini-pro'); + const selector = new LlmPromptSelector(registry); await selector.selectPrompts(['test'], sampleIndex); const call = (provider.complete as ReturnType).mock.calls[0]![0] as CompletionOptions; - expect(call.model).toBe('gemini-pro'); + expect(call.model).toBeUndefined(); + }); + + it('forwards an abort signal into complete (for the gate timeout)', async () => { + const provider = makeMockProvider('{ "selectedNames": [], "reasoning": "" }'); + const registry = makeRegistry(provider); + const selector = new LlmPromptSelector(registry); + const ac = new AbortController(); + + await selector.selectPrompts(['test'], sampleIndex, undefined, ac.signal); + + const call = (provider.complete as ReturnType).mock.calls[0]![0] as CompletionOptions; + expect(call.signal).toBe(ac.signal); }); it('throws when no heavy provider is available', async () => { diff --git a/src/mcplocal/tests/plugin-gate.test.ts b/src/mcplocal/tests/plugin-gate.test.ts index 72893af..f33c195 100644 --- a/src/mcplocal/tests/plugin-gate.test.ts +++ b/src/mcplocal/tests/plugin-gate.test.ts @@ -71,6 +71,8 @@ function setupPluginRouter(opts: { prompts?: typeof samplePrompts; withLlm?: boolean; llmResponse?: string; + /** When set, the heavy provider's complete() rejects — exercises the loud fallback. */ + llmError?: boolean; byteBudget?: number; } = {}): { router: McpRouter; mcpdClient: McpdClient } { const router = new McpRouter(); @@ -83,12 +85,14 @@ function setupPluginRouter(opts: { providerRegistry = new ProviderRegistry(); const mockProvider: LlmProvider = { name: 'mock-heavy', - complete: vi.fn().mockResolvedValue({ - content: opts.llmResponse ?? '{ "selectedNames": ["zigbee-pairing"], "reasoning": "User is working with zigbee" }', - toolCalls: [], - usage: { promptTokens: 100, completionTokens: 50, totalTokens: 150 }, - finishReason: 'stop', - } satisfies CompletionResult), + complete: opts.llmError + ? vi.fn().mockRejectedValue(new Error('upstream 400: model not found')) + : vi.fn().mockResolvedValue({ + content: opts.llmResponse ?? '{ "selectedNames": ["zigbee-pairing"], "reasoning": "User is working with zigbee" }', + toolCalls: [], + usage: { promptTokens: 100, completionTokens: 50, totalTokens: 150 }, + finishReason: 'stop', + } satisfies CompletionResult), listModels: vi.fn().mockResolvedValue([]), isAvailable: vi.fn().mockResolvedValue(true), }; @@ -195,6 +199,25 @@ describe('Gate Plugin via setPlugin()', () => { }); describe('begin_session', () => { + it('falls back with a visible warning when the LLM selector errors (no silent degrade)', async () => { + const { router } = setupPluginRouter({ withLlm: true, llmError: true }); + await router.route({ jsonrpc: '2.0', id: 1, method: 'initialize' }, { sessionId: 's1' }); + + const res = await router.route( + { jsonrpc: '2.0', id: 2, method: 'tools/call', params: { name: 'begin_session', arguments: { tags: ['zigbee', 'pairing'] } } }, + { sessionId: 's1' }, + ); + + // Fell back — the call succeeded rather than failing/hanging … + expect(res.error).toBeUndefined(); + const text = ((res.result as { content: Array<{ text: string }> }).content[0]!.text); + // … and the degradation is visible, not silent … + expect(text).toContain('Smart prompt-selection unavailable'); + // … while deterministic tag matching still returned relevant prompts. + expect(text).toContain('common-mistakes'); + expect(text).toContain('zigbee-pairing'); + }); + it('returns matched prompts with keyword matching', async () => { const { router } = setupPluginRouter(); await router.route({ jsonrpc: '2.0', id: 1, method: 'initialize' }, { sessionId: 's1' }); diff --git a/src/mcplocal/tests/with-timeout.test.ts b/src/mcplocal/tests/with-timeout.test.ts new file mode 100644 index 0000000..f58ab99 --- /dev/null +++ b/src/mcplocal/tests/with-timeout.test.ts @@ -0,0 +1,35 @@ +import { describe, it, expect } from 'vitest'; +import { withTimeout, TimeoutError } from '../src/util/with-timeout.js'; + +describe('withTimeout', () => { + it('resolves when the operation finishes in time', async () => { + const out = await withTimeout(async () => 'ok', 100, 'fast op'); + expect(out).toBe('ok'); + }); + + it('rejects with TimeoutError when the operation hangs', async () => { + const start = Date.now(); + await expect( + withTimeout(() => new Promise(() => { /* never resolves */ }), 50, 'hang op'), + ).rejects.toBeInstanceOf(TimeoutError); + // Returned promptly, not after the (never) inner promise. + expect(Date.now() - start).toBeLessThan(1000); + }); + + it('aborts the signal on timeout (fetch-based providers can cancel)', async () => { + let aborted = false; + await expect( + withTimeout((signal) => { + signal.addEventListener('abort', () => { aborted = true; }); + return new Promise(() => { /* never */ }); + }, 50, 'abort op'), + ).rejects.toBeInstanceOf(TimeoutError); + expect(aborted).toBe(true); + }); + + it('propagates the operation error (not a timeout) when it rejects fast', async () => { + await expect( + withTimeout(async () => { throw new Error('boom'); }, 100, 'err op'), + ).rejects.toThrow('boom'); + }); +});