Repository navigation
feat(health): degrade on rolling tool-call failure rate (Spec 113-d) - #1504
Merged
Merged
Conversation
…te (Spec 113-d) A connected server whose tool calls mostly fail still reported healthy, because health only looked at connection and OAuth state. Keep a rolling 5 min per-server window of call outcomes and report degraded when at least 5 counted calls were made and more than 50% failed. - new internal/upstream/callstats: 30x10s bucketed window, registry, and a denylist predicate (isError results, limiter/generation refusals, caller cancellation and auth-required errors are not counted) - upstream.Manager records outcomes at the dispatch choke point (Manager.callTool) and for code_execution via RecordClientCallOutcome; the window is dropped with the server in RemoveServer - health.CalculateHealth wraps the existing calculator: only a ready, enabled, healthy result is downgraded, so every existing non-healthy outcome keeps precedence - all three HealthCalculatorInput construction sites read Manager.CallStats; a drift-guard test keeps them in step - debounced servers.changed nudge when the verdict flips, including passive recovery when failures age out of the window
Deploying mcpproxy-docs with
|
| Latest commit: |
b4dad46
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1f5896d9.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://113-d-health-call-failure-ra.mcpproxy-docs.pages.dev |
…all window on client replacement, guard stale notifier timers (Spec 113-d review r1)
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 37439812051 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Dumbris
enabled auto-merge (squash)
October 6, 2026 17:24
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
2 participants
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.
Problem
A connected upstream whose tool calls mostly fail still reported
healthy: health looked only at connection and OAuth state. Gap G7 of the Anthropic MCP-proxy talk audit (Spec 113, FR-060..FR-068).What changed
internal/upstream/callstats: a 30 x 10 s bucketed rolling window (5 min) per server, a registry, and a denylist predicateClassify. Not counted at all:isErrortool results, limiter rejections,ErrConnectionGenerationChanged, caller cancellation (including the plain-text "context cancelled" form), auth-required errors. Everything else returned by the dispatch is a counted failure; a normal result is a counted success. Proxy policy/quarantine refusals and argument validation happen before dispatch and never reach the recorder.upstream.Managerrecords atcallTool(covers direct surface,call_tool_*, REST) andcode_executionrecords viaRecordClientCallOutcome(it bypasses the manager). The window and notifier state are dropped inRemoveServerinside the same critical section as the client delete. Nothing inmanaged/client.goorActivityRecord.health.CalculateHealthis now a wrapper: only a ready, enabled, healthy result is downgraded todegradedwhencalls >= 5andfailures/calls > 0.5. Summary: "N of M tool calls failed in the last 5 min"; detail names the dominant failure kind; actionview_logs. Disabled, quarantined, connecting, unhealthy, OAuth and refresh outcomes all take precedence (table tests). Thresholds are constants (health.CallFailureWindow,CallFailureMinSamples,CallFailureRatio); no config field.HealthCalculatorInputconstruction sites (runtime.go,server.go,mcp.go) readManager.CallStats; a drift-guard test fails if a site stops assigningCallsInWindow.Manager.SetCallHealthObserver, wired inruntime/lifecycle.gotoemitServersChanged("call_failure_rate")(already coalesced). Debounced to one per 10 s per server, with a per-bucket recheck timer while the window holds calls so passive recovery or expiry-driven degradation also notifies.docs/designs/2025-12-10-unified-health-status.md.Tests run
go test -raceoninternal/health,internal/upstream/...,internal/runtime/...: pass.go test -race -tags server -skip "E2E|Binary|MCPProtocol|TestInfoEndpoint|TestGracefulShutdownNoPanic|TestSocketInfoEndpoint" ./internal/server/... ./internal/httpapi/...: pass.TestGetAllServers_HealthDegradesOnCallFailureRate(runtime projection, 6 of 10 -> degraded), window expiry incallstatsand notifier tests with an injected clock.-tags serverbuilds; golangci-lint (both runs): no findings in touched files (the local linter reports 15/18 pre-existing issues in untouched files).scripts/test-api-e2e.shon an isolated port (18468): 65/66 pass. The one failure, "Audit log: server-edition binary present", needs a prebuiltmcpproxy-serverbinary (environmental, unrelated to this change).make swagger.Assumptions
internal/health(notcallstats) so the calculator does not import the managed-client dependency tree;callstatstakes the window length as its own constant of the same value.HealthCalculatorInputgets a third field,DominantCallFailureKind, so the detail text can name the kind without health importing callstats.Spec: #1495
Review: opencode github-copilot/gpt-6.1-sol, 5 rounds, clean; earlier rounds fixed text-form cancellation, expiry-driven notification, in-flight resurrection of a removed window and a late-drop wipe; the latest full re-review (3 chunks) found 3 mediums (text-cancel exclusion without a done caller ctx, call window not dropped on config-change client replacement, stale notifier timer callbacks), all fixed, incremental round CLEAN; follow-ups: none.