feat(upstream): request queueing + per-upstream concurrency limits (spec 093) - #959
Merged
Conversation
Related #955 Spec for Option A from the decision research: two-tier limiter at the managed-client choke point; max_concurrent_requests/queue_size/ queue_timeout (global + per-server), 0=unlimited opt-in defaults, hot-reloadable; isError/429 shed semantics. Includes the decision report under docs/research/.
Deploying mcpproxy-docs with
|
| Latest commit: |
86cbf02
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://961dcf38.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://093-concurrency-limits.mcpproxy-docs.pages.dev |
…ic reload generation, origin-independent shed seam Related #955 - FR-008/009: never queue under manager locks; prompt-fail queued calls on disable/remove - FR-020: tri-state per-server semantics (absent=inherit, 0=none, positive=override) - FR-005: execution timeout starts after admission (replay restructure required) - FR-021: atomic limit generation for hot reload - FR-004: queue_timeout is one absolute deadline across both tiers - FR-011: typed rejection identity preserved through REST path - FR-012/013: origin-independent rejection seam + full activity-status consumer contract (schema, filters, summaries, aggregates, UI); reason+scope metadata - FR-010: global-limit sheds must not blame a server - FR-022: env overrides global-only - SC-002: measurable compatibility/benchmark criteria
…-after-disable race, three-scope config model, no unbounded queue Related #955 - FR-021: occupancy shared across generations (lowered caps block new admissions until drain; grandfathered over-cap transient and bounded) - FR-009: atomic lifecycle check at admission (tombstone) + limiter retirement after drain; re-add gets fresh capacity without double-count - FR-020: three separately named scopes (global aggregate / per-server default set / per-server overrides), tri-state per setting for all three fields - FR-011: removed 'unlimited queueing' clause; queues always bounded, queue_size 0 = no pending capacity
…shared occupancy Related #955
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 31244441914 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…pancy Related #955 New internal/upstream/limiter package: per-scope cap over a bounded FIFO wait queue, acquired with ONE absolute deadline shared across tiers (FR-004). Typed *LimitError carries scope/reason/server/limit/retry-after so the shed seams (MCP isError, REST 429, activity "rejected") can map it end-to-end. ## Changes - Limiter: FIFO waiter list over a resizable cap (not x/sync/semaphore — a semaphore's capacity is fixed, but FR-021 needs occupancy shared across generations on hot reload) - Registry: atomic per-server + global instances, Apply() publishes one generation, retirement tombstone fails queued calls promptly (FR-009) - nil limiter / zero max = passthrough; queue_size 0 = shed at the cap ## Testing - go test -race -count=5 ./internal/upstream/limiter/... — pass - covers FIFO order, instant queue-full reject, absolute-deadline timeout, caller-cancel vs shed, hot-swap lower/raise under load, retire races
Related #955 Adds the config surface for spec 093's request queueing: a global aggregate limiter, an optional per-server default set, and per-server overrides — three separately named scopes each carrying max_concurrent_requests / queue_size / queue_timeout with uniform tri-state semantics (FR-020). Everything is off by default, so an untouched config serializes unchanged. ## Changes - config: global fields + server_concurrency_defaults + per-server overrides; ResolveGlobalConcurrency / ResolveServerConcurrency / ResolveQueueBudget (one absolute deadline across tiers, FR-004) - validation per resolved scope, naming scope + field (FR-023); an explicit per-server max 0 stays a valid opt-out - DetectConfigChanges clauses for the global fields and the default set (per-server rides the Servers DeepEqual) - env overrides for the global scope only: MCPPROXY_MAX_CONCURRENT_REQUESTS, MCPPROXY_QUEUE_SIZE, MCPPROXY_QUEUE_TIMEOUT (FR-022) - merge/copy of the per-server tri-state pointers; make swagger - docs/configuration.md: new section + server fields + env table ## Testing - go test -race ./internal/config/... ./internal/runtime/... ./internal/upstream/... — pass - go test -tags server ./internal/serveredition/... -race — pass - golangci-lint (v2, .github/.golangci.yml) — 0 issues
…e point Related #955 Admission control now runs inside managed.Client.CallTool, above coreClient.CallTool where the call_tool_timeout context is created — so queue waiting never consumes execution budget (FR-005) and every in-process dispatch path is bounded by the same limits (FR-003): call_tool_* variants, the REST tool-call endpoint, sandboxed code_execution and activity replay all funnel through it. ListTools and the health-check Ping are deliberately left unwrapped (FR-007). ## Changes - internal/upstream/limiter/observer.go: Rejection payload + Observer seam (origin-independent, FR-012/FR-013) and Registry.Active fast path - internal/upstream/managed/admission.go: per-server-then-global acquire under ONE absolute queue deadline resolved from ResolveQueueBudget - internal/upstream/concurrency.go: Manager owns the limiter registry; Apply publishes one generation on SetGlobalConfig (FR-021), add/update republishes one server, remove/disable/quarantine retires it (FR-009) - internal/upstream/manager.go: CRITICAL lock fix (FR-008) — CallTool snapshots the target client under m.mu.RLock and RELEASES it before reconnect-on-use, admission and the upstream call; a queued call can no longer stall AddServer/RemoveServer/config reload. Limiter rejections are returned verbatim so the typed identity survives (FR-011) ## Testing - upstream: queued-call-vs-server-management (FR-008), prompt fail on remove (FR-009), hot reload shares occupancy, disabled/quarantined own no limiter, zero-config allocates nothing, -race churn deadlock guard - managed: passthrough, instant queue-full shed, absolute-deadline timeout, global scope never names a server, caller cancel is not a shed, cap raise admits mid-queue, queue-budget hot reload
…vity Related #955 A concurrency-limiter shed is now a first-class, attributable outcome on every surface instead of an anonymous upstream failure. ## Shed surfacing - MCP (call_tool_* variants, legacy handler, direct-routing mode): a retry-friendly isError tool RESULT, never a protocol error that could abort an agent session (FR-010). The wording lives on LimitError.UserMessage so the MCP text, the REST body and the log all read identically; the global-scope branch never names a server. - REST: the typed rejection survives CallToolDirect through a per-call capture box installed in the context — the IsError branch used to flatten it to a string, which made 429 mapping impossible (FR-011). internal/httpapi answers 429 with Retry-After derived from the shedding scope's effective queue_timeout (delta-seconds, rounded up). - Activity: new "rejected" status emitted from the limiter observer, BELOW the MCP dispatch layer, so code_execution and activity replay are covered by construction (FR-012). Metadata carries rejection_reason (queue_full|queue_timeout), rejection_scope (server|global), limit and retry_after_ms. Propagated through the storage vocabulary, activity summaries (rejected_count), usage aggregation (counted like blocked — never inflates calls, latency or the executed-call timeline), the type generator, api.ts, and the Web UI filter/badge/KPI. - Metrics: mcpproxy_tool_calls_rejected_total{server,reason,scope} plus mcpproxy_concurrency_active / _queue_depth{scope,server} gauges, sampled every 10s by the observability bridge (FR-013). ## Also - Activity replay no longer creates its CallToolTimeout context before dispatch: queue wait was being subtracted from the execution budget (FR-005). The execution timeout starts after admission, inside core.Client.CallTool. - Per-server concurrency fields now round-trip through UpstreamRecord — SaveConfiguration rebuilds the JSON server list from those records, so a REST/UI-set limit would otherwise be wiped on the next save. ## Testing - server: message wording per scope, isError shape, server-unavailable is not a shed, capture box, shedDispatchError unwrapping, and code_execution admission coverage through upstreamToolCaller - httpapi: 429 + Retry-After for both scopes, 500 for server-unavailable, delta-seconds rounding - runtime: rejected activity record + metadata, usage aggregate does not inflate calls, source mapping, AST guard that replay never re-introduces a pre-dispatch deadline
Related #955 The existing spec-093 tests are synthetic — they drive the limiter or the shed seams directly. This adds the User Story 1 / 2 "independent test" against a REAL streamable-HTTP upstream whose tool blocks until the test releases it, so the whole chain is exercised: config resolution → registry generation → managed-client admission → live upstream. - max_concurrent_requests=1, queue_size=1: call 1 runs, call 2 queues and then succeeds, call 3 is shed. The upstream's own peak-concurrency counter asserts it never observed two simultaneous requests (SC-001). - The queue-full shed lands in under 100ms (SC-005) and carries the typed *limiter.LimitError identity the MCP/REST shed seams key off. - FR-003: the same saturated limiter sheds a call made through upstreamToolCaller, the sandboxed code-execution origin that never traverses handleCallToolVariant. - FR-004: a second case with queue_timeout=250ms proves a queued call is shed with ErrQueueTimeout instead of waiting out the upstream, and that RetryAfter carries the shedding scope's timeout. Both pin CI="" so they behave identically locally and in CI.
Related #955 FR-020 scope (c) documents the per-server overrides as "file/API-configured", but only the file half existed: config.ServerConfig carried max_concurrent_requests / queue_size / queue_timeout and the merge layer honoured them, while the REST DTO and the request structs did not mention them at all — so a limit could not be set or read back through POST/PATCH/GET /api/v1/servers. - contracts.Server gains the three tri-state fields; both converters map them (the generic-map path coerces int/int64/float64 so a JSON round-trip keeps an explicit 0, which means "opt this server out", distinct from an absent key meaning "inherit the default set"). - httpapi.AddServerRequest gains them with the established nil-preserve semantics: applied on create when present, and on PATCH an omitted key carries the existing pointer forward so an unrelated patch cannot wipe a configured limit. - Regenerated frontend/src/types/contracts.ts via cmd/generate-types and oas/ via make swagger. - docs/configuration.md: REST examples for the per-server limits. ## Testing - contracts: tri-state round-trip through both converters, incl. explicit 0 and the float64 JSON shape - httpapi: PATCH sets/preserves each field, explicit 0 survives, GET echoes all three
…face Related #955 Spec 093 FR-012 requires the new "rejected" activity status to be propagated through the FULL consumer contract, not just storage and the Web UI. Three surfaces still had the old three-value vocabulary: - GET /api/v1/activity/usage validated ?status against success|error|blocked and 400'd on "rejected" — which also made the usageMatchesStatus "rejected" branch unreachable dead code. - `mcpproxy activity list|export --status rejected` failed the CLI's own whitelist, and formatStatusIcon/the watch renderer had no case for it (a shed rendered as "?" with no explanation). - The TUI activity view fell through to the neutral style. Rendering choice: a shed is backpressure, not a failure, so it renders degraded (⏸ / "REJECTED (concurrency limit)") rather than in the error bucket. ## Testing - httpapi: ?status=rejected returns 200 and filters to the shed tool
Member
Author
Implementation pushed — spec is now fully implemented7 commits on top of the spec (limiter package → config surface → enforcement → shed semantics → REST exposure → e2e coverage). What landed
Local verification (all on this branch)
Known scope calls: no Web-UI form control for per-server limits yet (REST + types are wired; no FR requires the form), CLI debug client documented out of scope. Related #955 🤖 Generated with Claude Code |
Related #955 Codex round 2, P1 findings against the spec-093 limiter. ## Changes - Retirement now tombstones the registry entry instead of deleting it, so a call that snapshotted its client before the server was disabled is refused with server-unavailable at admission rather than sailing through an absent limiter (FR-009). A re-added server still gets a fresh instance; drained tombstones are pruned on the next generation publish. - Grant-vs-retire is atomic under the limiter's own lock: a waiter granted a slot moments before Retire re-checks retirement after waking and hands the slot back, instead of running against a retired scope (FR-009). - Every eligible scope owns an instance even when it caps nothing, so enabling a cap on a previously unlimited scope inherits the grandfathered occupancy instead of starting at zero and admitting a second cap's worth on top of the calls already in flight (FR-021 shared occupancy). - Admission resolves both limiter tiers, both scopes' reported limits and the single absolute queue deadline from ONE published generation behind one atomic pointer, so no admission can mix generations (FR-021). The queue budget moves with it: config.ResolveQueueBudget was a second copy of the FR-004 rule that no longer governed anything. ## Testing - Deterministic grant-vs-retire interleaving via a waiter wake hook - Registry: admission-after-retire refused, unconfigured scopes still count occupancy, concurrent Apply/Acquire storm asserting single-generation reads - Manager: hot-enabling a cap below the live occupancy admits nothing until drain
Related #955 Codex round 2, P2-1 and P2-2. ## Changes - ReplayToolCall snapshots its collaborators under r.mu and releases the lock before dispatch. It used to hold the read lock across admission and execution, so one replay queued behind a concurrency limit stalled ApplyConfig and every other writer for the whole queue duration (FR-008). - Replay takes the caller's context instead of substituting context.Background(), so a client that disconnects while the replay waits for a slot releases it immediately (FR-005). Its request source is stamped internal: replay never crossed an external surface. - A shed replay returns the typed *limiter.LimitError instead of flattening it into the record's Error field and reporting success, and the REST handler maps it to 429 + Retry-After like the tool-call endpoint (FR-011). ## Testing - httpapi: replay endpoint answers 429 + Retry-After with success:false - runtime: structural guard that the lock is released before dispatch
Related #955 Codex round 2, P2-3. Rejection metrics and activity rows both rode the general event bus, whose publish drops events for any subscriber whose channel is full — and a burst of sheds is the one load that fills it, so the numbers thinned out exactly when an operator would be reading them. ## Changes - The rejection counter is now incremented synchronously at the rejection site through a sink the observability bridge installs, and the bus-driven increment is gone so a shed is counted once (FR-013). - The "rejected" activity row is written synchronously by the activity service on the rejecting goroutine; the bus copy remains for live subscribers only, and its handler no longer persists (FR-012). The write is skipped once the service has stopped, so nothing lands after shutdown. - docs/configuration.md states the guarantee: exact counter, exactly one row per shed, no origin bypass. ## Testing - server: sink counts synchronously, normalises empty labels, and the bus event does not add a second increment - runtime: rejection row written once even when the bus copy is also handled
Related #955 Codex round 2, P2-4. A shed dispatched through an MCP tool-call variant wrote two rows — the limiter's canonical tool_call rejection plus the handler's internal_tool_call echo — and the default filter only hid SUCCESSFUL internal call_tool_* rows, so activity listings and the summary's rejected count showed a saturated proxy as twice as saturated as it was. ## Changes - The default filter now also hides REJECTED internal call_tool_* rows: they are always covered by the limiter's canonical record, which every origin produces. Failed ones stay visible (they have no tool_call counterpart), and include_call_tool=true still shows everything. - The code_execution and replay origins never reach the variant handler, so their shed already produced the canonical row only. ## Testing - storage: exactly one rejected row survives the default filter; the echo reappears with include_call_tool=true; failed variants still visible - server: the count of rejected internal_tool_call emitters is pinned to the single variant handler, so a new dispatch path cannot re-introduce the pair
…call Related #955 Codex round 2, P3-1. Two dispatch paths mislabelled the origin on the "rejected" record (and on every other activity row they produce). ## Changes - POST /api/v1/tools/call stamped every caller as CLI, overwriting the REST source the middleware had set — so a Web-UI or tray tool call was logged as a CLI one. The surface header those clients already send now decides: "cli/<version>" is CLI, anything else is REST. - A sandboxed script's upstream call inherited the outer surface's context, so a shed inside code_execution was attributed to the MCP client that started the script. It is stamped internal, matching how replay is attributed. ## Testing - httpapi: surface-header classification incl. tray/webui/unknown clients - server: a shed inside a script reaches the observer with the internal source even when the outer context says MCP
Dumbris
changed the title
Related #955 Codex round 3, P1-A and P1-B. P1-A: limits lived on the limiter instance and Apply mutated them BEFORE storing the new generation, while the admission decision read those mutable values rather than the snapshot it had resolved. An admission that resolved an UNCAPPED generation — wait budget 0, because nothing limits it — could then be decided against a freshly enabled cap, find the scope saturated, and park in the queue with no deadline at all, released only by the caller's context. P1-B: a drained tombstone was pruned at the next Apply. Absence of a scope means "unlimited", so pruning let a caller that had resolved its client before the server was disabled sail through admission instead of being refused. ## Changes - A Limiter owns occupancy only; it no longer stores limits. Every admission decision — capacity, pending capacity, and what the rejection reports — is made against the values handed to it from the ONE generation the admission resolved, so no value can change underneath it (FR-021). - publishLocked stores the generation FIRST and re-evaluates every wait queue against it afterwards, which is what keeps "a raise admits eligible waiters immediately" and "a lowered cap admits nothing until occupancy drains" both true. Handing a freed slot to a queued waiter uses the newest published cap (a single-scope decision, so nothing to mix). - A capped generation always publishes a positive wait budget, so a queued call can never sit without a deadline. - Tombstones are never pruned (FR-009). A re-added server replaces its tombstone, so the map holds at most one entry per distinct server name the process has ever configured. ## Testing - an admission resolved against an uncapped generation is admitted rather than queued after a cap is published (the hang, reproduced) - a capped generation always yields a wait budget, per-server or global - snapshot → retire → three reload cycles → admission still server_unavailable, and the tombstone survives - Apply/Acquire storm: consistent (limits, budget) per generation and peak occupancy never exceeds the largest cap published
…rrier Related #955 Codex round 3, P1-C. The rejection row is written on the rejecting caller's goroutine, but it only checked the stopped flag and then wrote — it never joined workersWG — so Stop could return, and Runtime.Close could resolve the shutdown marker and close BBolt, with that write still in flight (Spec 080 FR-010). ## Changes - enterWrite checks stopped and registers in workersWG under the one lock Stop coordinates with, in that order, so a write either completes before Stop returns or never touches the DB. It matches how the retention, usage-flush and async-detection writers register. - Stop now waits on workersWG unconditionally. Its early return when Start never ran skipped the wait entirely, and these writers exist whether or not the event loop was started. ## Testing - a write registered before Stop begins is finished before Stop returns (fails against the old early-return path) - rejections raced against a stopped service write nothing
…n link Related #955 The docs site includes only docs/configuration/**, not the root docs/configuration.md, so the activity-commands link into the root reference failed the site build (onBrokenLinks: throw). ## Changes - docs/configuration/config-file.md: a "Concurrency Limits & Request Queueing" section covering the three scopes, tri-state per-server semantics, 0 = unlimited, min(per-server, global), shed behavior and hot reload, with a pointer to the repository's full reference for validation and metrics depth. - docs/configuration/environment-variables.md: MCPPROXY_MAX_CONCURRENT_REQUESTS, MCPPROXY_QUEUE_SIZE and MCPPROXY_QUEUE_TIMEOUT, noting they override the global aggregate limiter only. - The activity-commands link now targets the on-site section. ## Testing - cd website && npm run build: SUCCESS, no broken-link or broken-anchor error; the rendered link resolves to /configuration/config-file#concurrency-limits--request-queueing and that anchor exists on the built page
Related #955 Two writers added the same site docs concurrently; keep one section per page (the config-file.md version with the JSON example and option table).
Member
Author
Cross-model review status: 5/5 rounds used — 2 findings outstandingCodex reviewed this PR through 5 fix→re-review rounds (3 on the spec, 2 on the implementation; 18 code findings fixed and re-verified). The final round left two narrow findings, documented here per the review-cap policy:
Both are small, localized fixes in Related #955 🤖 Generated with Claude Code |
Related #955 Codex round 4. P1: publishing a raise is necessarily two steps — store the generation, then re-grant the queue — and the fast path admitted on free capacity without ever consulting the queue. A call arriving in that window took the slot the raise had just created, ahead of a waiter that had been in line for the whole reload. The same hole is reachable any time an admission's generation is more permissive than the one the waiters were last measured against. P2: the queue-budget fallback was applied once at the end rather than per scope, so a capped scope with no queue_timeout of its own contributed nothing whenever the other scope named one — a server capped with no timeout beside a global 60s inherited 60s instead of min(30s, 60s). The rejection that scope produced also advertised Retry-After 0, which the REST surface renders as one second for a wait that is really thirty. ## Changes - acquire drains the queue against the current limits before judging this call, and the fast path is taken only when nobody is waiting. Draining first keeps a raise from stranding waiters and means the queue-full check below judges capacity that is genuinely spare, so nothing is shed while a slot sits free. The re-grant on publish stays as it was — no thundering herd. - grantLocked returns early on an empty queue, keeping the uncontended admission path free of even a limits read. - effectiveQueueTimeout resolves each ACTIVE scope's timeout independently (its value, else the fallback); the shared deadline is the min of those, and a shed reports its own scope's effective timeout as Retry-After. ## Testing - a raise that creates exactly one slot gives it to the queued call, with a newcomer racing for it in the pre-re-grant window - a permissive generation cannot overtake an existing queue, and the two queued calls are served in arrival order - budget table extended to 11 cases incl. both mixed named/fallback directions - a shed from a scope capped without a timeout reports the fallback, not 0
Member
Author
|
✅ Cross-model review complete — CLEAN. The two outstanding findings were fixed in Related #955 🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
Summary
Spec for native concurrency limiting decided in the #955 investigation (Option A of the decision report, included under
docs/research/):max_concurrent_requests, bounded FIFOqueue_size,queue_timeout— configured inmcp_config.json, per-server overrides, env overrides, hot-reloadable. 0 = unlimited: pure opt-in, zero behavior change on upgrade.code_executionand activity-replay paths that bypass the manager layer (verified in the audit).tools/call→isError:trueretryable tool result; REST → 429 + Retry-After; activity log statusrejected; rejection counters + queue-depth metrics.Related #955
Review notes
Spec only — no code. Key verified findings: all four dispatch paths converge on one function while code_execution/replay bypass
Manager.CallTool; stdio upstreams are genuinely multiplexed by mcp-go (limit ≠ 1); queue wait deliberately does not consumecall_tool_timeout;x/syncis already a dependency.Open maintainer decisions listed in the report (stdio default value, -32029 protocol-error convention, per-user fairness timing, health integration).
🤖 Generated with Claude Code