fix: MCP client poison recovery and per-tool-call timeout#53
Merged
Conversation
One tool call that consumed the run budget and expired mid-read poisoned the shared MCP client, and nothing ever recovered it — the registry held one client per server for the process lifetime, so every subsequent tool call from every agent short-circuited with ErrPoisoned until a manual `leather serve` restart (a 7.5-hour production outage). Two fixes: - Poison recovery: Registry.Recreate closes the poisoned client and starts a fresh one under a mutex (serialized so concurrent callers don't race multiple restarts). On mcp.ErrPoisoned the executor recreates once and retries, so a poisoned transport self-heals on the next call. - Per-tool-call timeout: a single run-level timeout governed every LLM call AND every tool call, so a slow tool ate the whole run budget and expired the shared read. New `tool_timeout` config (global default 600s; env LEATHER_TOOL_TIMEOUT; flag --tool-timeout), overridable per agent via front-matter / lifecycle `tool_timeout:`, wraps each tool call in a child deadline. A tool exceeding it fails one call cleanly while the run continues; 0 disables it. Tests cover the poison→recreate→success cycle (incl. concurrent recreation), execMCP recovery, and the per-tool timeout bounding a single call without consuming the run budget.
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
1 participant
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.
Summary
Implements
plans/02-mcp-client-resilience.md(Steps 1 & 2 — the runtime fixes). Steps 3 & 4 are config-only changes in the separatetannery/sh-webworkspace repo and are out of scope for this PR.Serve-log evidence from 2026-07-15/16 showed one slow tool call (
github-analytics-auto, an 89-repo fetch) hit the 900s run deadline mid-read. That poisoned the shared MCP client, and because the registry holds one client per server for the process lifetime with no recovery path, every subsequent tool call from every agent failed withclient poisoned by prior read timeoutuntil a manual restart — a 7.5-hour outage.Changes
Poison recovery (
internal/mcp/registry.go,internal/tool/executor.go)Registry.Recreate(ctx, name, stale)closes the poisoned client and starts a fresh one, swapping it into the registry under a mutex. Serialized so concurrent agents don't race multiple restarts; if another caller already recreated, the fresh client is reused. Theclientsmap is now guarded for concurrentGet/Recreate.execMCPrecreates the client once and retries onmcp.ErrPoisoned, so a poisoned transport self-heals on the next call instead of requiring aleather serverestart. The poison mechanism itself is unchanged — it correctly protects the corrupted transport.Per-tool-call timeout (
internal/runner/runner.go, config plumbing)tool_timeoutconfig: global default600s(envLEATHER_TOOL_TIMEOUT, flag--tool-timeout), overridable per agent via front-matter / lifecycletool_timeout:.context.WithTimeout, distinct from the run-leveltimeoutbudget. A tool exceeding it fails a single call cleanly (tool X exceeded tool_timeout …) while the run continues.0disables the per-tool deadline.leather doctor.Tests
TestRegistry_RecreateAfterPoison— full poison →ErrPoisoned→ recreate → success cycle.TestRegistry_RecreateConcurrent— 8 concurrent callers trigger exactly one restart.TestExecMCP_RecoversFromPoison— executor recovery path end-to-end.TestRunner_PerToolTimeoutFailsOneCall— per-tool timeout bounds one call, run continues, run budget not consumed.TestParseFrontMatter_Full—tool_timeout:front-matter parsing.Validation
go build ./...,go vet ./...,gofmt,golangci-lint run(0 issues)go test ./...passes;-raceclean oninternal/mcpandinternal/toolleather doctorshowstool_timeout 10m0s default