fix(mcplocal): one LLM budget per stage, so a loop cannot multiply it
A per-call timeout bounds one call. It does not bound a stage that calls the
LLM in a loop -- and summarize-tree does worse than loop: buildTree recurses to
maxDepth (3 by default) and iterates per section at every level, then
groupSections iterates again. Hundreds of sequential calls are reachable, so a
10s per-call cap is a multiplier, not a bound.
StageBudget is one wall-clock budget shared across everything a single stage
invocation does. Once it is gone the stage takes its deterministic path --
first-line excerpts, numbered pages -- and says so. The executor builds one per
stage from config.budgetMs ?? MCPCTL_STAGE_LLM_BUDGET_MS (30s) and disposes it
in a finally, so a long-lived mcplocal never accumulates timers.
Two details that matter more than they look:
- A warm cache is never budget-gated. cachedSummarize and generatePageTitles
used ctx.cache.getOrCompute, which makes the budget check impossible to
place correctly; split into get/set so a cached summary -- which costs
nothing -- is still returned when the budget is spent.
- "No LLM configured" is NOT a degradation. It is a deliberate choice, and
numbered pages are the expected output there, so it gets no warning. Only
failures, timeouts and exhausted budgets do. Crying wolf on a working
configuration would train people to ignore the notice.
summarize-tree's single-block path previously had no try/catch at all, so a
failure escaped the stage entirely and executor.ts discarded the whole stage's
work. It now degrades like the others.
Degradation is reported three ways, all from util/degrade.ts: the reason in the
content behind the established "⚠ <feature> unavailable (<reason>)" prefix, the
count of affected sections (the recursion can hit one exhausted budget dozens of
times, so the user needs the reason once plus what it cost), and
degraded/degradedReason on the stage_execution audit event -- previously a
degradation was invisible in the trace.
docs/reliability.md now names ONE helper rather than a list of compliant call
sites. That list is exactly how this drifted: the doc named the gate and
llm/pagination.ts, and the newer stages never joined it.
Tests: the 20-section recursion completes in ~300ms against a 300ms budget;
a warm cache still serves real titles with the budget exhausted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GqMidYEGUJG5fxeoTELBu2
This commit is contained in:
@@ -19,10 +19,47 @@ and report the degradation — never hang and never degrade silently.**
|
||||
(<reason>)…` 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.
|
||||
**One implementation:** [`util/degrade.ts`](../src/mcplocal/src/util/degrade.ts)'s
|
||||
`bounded()`. It cannot throw — the caller always gets a value or a reason — so
|
||||
the deterministic fallback is unconditional rather than something a `catch`
|
||||
block has to remember. `degradationNotice()` produces the `⚠ … unavailable
|
||||
(reason). hint` wording, and `degradationAudit()` the `degraded`/`degradedReason`
|
||||
payload, so every degradation reads the same whichever subsystem produced it.
|
||||
|
||||
**Nothing optional is unbounded by construction.** The budget lives in
|
||||
`LLMProviderAdapter.complete()` (`proxymodel/llm-adapter.ts`), not at each call
|
||||
site, so a stage that passes no options is still bounded and a stage written
|
||||
next year inherits the guarantee. One budget spans the whole failover chain —
|
||||
a per-provider timeout would make the worst case N × timeout.
|
||||
|
||||
| Knob | Default | Bounds |
|
||||
|---|---|---|
|
||||
| `MCPCTL_LLM_CALL_BUDGET_MS` | 20s | one `ctx.llm.complete()`, failover included |
|
||||
| `MCPCTL_LLM_PROVIDER_TIMEOUT_MS` | 10s | a single provider attempt inside that budget |
|
||||
| `MCPCTL_STAGE_LLM_BUDGET_MS` | 30s | all LLM work in one stage invocation |
|
||||
| `MCPCTL_GATE_LLM_TIMEOUT_MS` | 8s | the gate's `begin_session` prompt selection |
|
||||
| `MCPCTL_PAGINATION_LLM_TIMEOUT_MS` | 10s | pagination's smart index (`llm/pagination.ts`) |
|
||||
|
||||
The **stage** budget exists because a per-call timeout multiplies rather than
|
||||
bounds when a stage loops: `summarize-tree` recurses to `maxDepth` (3) and loops
|
||||
per section at every level, so hundreds of sequential calls are reachable. One
|
||||
budget is shared across the whole recursion; when it is gone the stage switches
|
||||
to first-line excerpts and says so. A **warm cache is never budget-gated** — a
|
||||
cached summary costs nothing, so an exhausted budget must not degrade a result
|
||||
we already hold.
|
||||
|
||||
`read_prompts` is LLM-free by design.
|
||||
|
||||
### Why this is written down twice
|
||||
|
||||
This document stated the principle while `proxymodel/stages/paginate.ts` awaited
|
||||
`ctx.llm.complete()` with no timeout at all. When a provider hung rather than
|
||||
erroring, the promise never settled, the tool call never returned, and the
|
||||
client waited out its own 1800s timeout — three such requests in production,
|
||||
misdiagnosed twice as an upstream "transport fault". The doc named the gate and
|
||||
`llm/pagination.ts` as the compliant sites, and the newer stages simply never
|
||||
joined the list. Naming **one** helper here, rather than a list of call sites,
|
||||
is what stops that drift recurring.
|
||||
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user