GeezerVison is showing github.com — fix(mcp_server): enforce max_concurrent_requests and request_timeout on tool calls by pcbeingused333 · Pull Request #906 · stackql/stackql
Conversation
pcbeingused333
commented
Nov 7, 2026
•
edited by jeffreyaven
Loading
Uh oh!
There was an error while loading. Please reload this page.
Part of #828: covers the first two bullets of "Required outcome" (bounded admission and request deadlines). The issue should stay open, see "Not in this PR" below.
Summary
server.max_concurrent_requests and server.request_timeout were accepted in config but never enforced: the semaphore was built and never acquired, and the timeout was never applied. This PR enforces both.
Original implementation and tests by @pcbeingused333. A maintainer follow-up commit on the same branch keeps the goal and changes where the limits are applied, for the reasons below.
Where the limits apply, and why
The first revision installed the limits as a receiving middleware around tools/call. mcp.NewServer installs the SDK's multi round-trip middleware itself, so a middleware added afterwards wraps it. For clients older than 2026-07-28 the approval prompt is awaited inside the tools/call, which gave three problems once an operator set the knobs:
request_timeout counted the time a user took to approve. An approval given after the timeout failed with context deadline exceeded and the approved statement never ran.- A slot stayed held while a prompt was open. With
max_concurrent_requests: 1, one unanswered prompt stalled every other tool call, server_info included. On a shared HTTP server that is a cheap way for one client to stall the rest.
- A call refused for lack of a slot never reached the tool gate, so it left no audit record and surfaced as a JSON-RPC protocol error.
The limits now live in a Backend decorator (limitedBackend in pkg/mcp_server/limits.go), applied below the tool gate:
- A call that the mode refuses, or that is waiting on approval, holds no slot and spends none of its deadline.
request_timeout is the budget for one backend call, queue wait included. max_concurrent_requests caps backend calls in flight across all clients; further calls queue.- Limit hits are tool errors (
isError) that name the setting and go through the gate's audit path. server busy means the call never ran. A mutation or lifecycle call that times out says it may already have taken effect.
validate_select_query no longer reports a limit hit as valid: false.server_info, ping and initialize are not limited.- A value of zero or less disables that limit, so a zero-valued config cannot deadlock.
limitedBackend is an unexported type built only by newLimitedBackend; nothing is exported from the package.
Behaviour change for existing deployments
Both limits remain off unless set. stackql mcp builds its config from an empty object, so the DefaultConfig() values (100 and 30s) do not reach the binary. A deployment that already sets either key will see it enforced for the first time.
Other changes
reverse_proxy backend: passes the request context to ExecContext / QueryContext, so the deadline applies there too (it was dropped before). It also returns a row iteration error instead of a short result.docs/mcp.md: new "Request limits" section. Field comments in pkg/mcp_server/config.go updated.
Review feedback addressed
- Timeout not effective on the reverse proxy backend: fixed as above.
TestQueuedToolCallReleasesPermitOnCancel passed with the release removed: replaced by tests that fail under that mutation.- No robot coverage: two scenarios added.
Not in this PR (still open on #828)
- Finite defaults for both settings.
- Explicit
http.Server timeouts and request body limit.
- Per-client fairness: the slot pool is shared by all clients of the process.
- Robot scenarios for the limits over the HTTP transport (the new ones use stdio).
Test plan
- Unit (
pkg/mcp_server/limits_test.go, rewritten on testing/synctest, no sleeps): concurrency bound, timeout ends the call and frees the slot, refusal when no slot frees, caller cancel while queued, write timeout wording, slot released on panic, non-positive limits leave the backend unwrapped. End to end through the in-process client: limit errors are audited tool errors, the timeout applies through the real wiring, and an open approval prompt (2025-06-18 client) holds no slot and spends no deadline.
- Mutation checks, each caught by the tests: slot never released, limit hit reported as a validation verdict, deadline not applied, limits moved back to the outer middleware.
- Robot (
test/robot/functional/mcp.robot):
MCP Stdio Request Timeout Ends Slow Call And Session Survives (new).MCP Stdio Concurrency Limit Admits Sequential Calls (new).MCP Stdio Startup Masks Credentials And Preserves Configuration: its config set request_timeout to 1ms and expected a live query to succeed, which only held while the setting was inert. It now uses 30s. This was the failing CI scenario on the first revision.
- Local:
golangci-lint v2.15.0 reports 0 issues; unit tests pass; the MCP robot suite passes apart from the version-string assertions that need a release-stamped binary.
Original description
Cause. max_concurrent_requests only built a semaphore.Weighted that was never acquired, and request_timeout was never applied to any request.
Fix. New receiving middleware (pkg/mcp_server/limits.go) registered before the tools. For tools/call it applies context.WithTimeout(request_timeout) and acquires a permit from the semaphore (waiting is cancellation-aware; permit released via defer on every exit path). Values <= 0 disable the respective control, so a zero-valued config cannot deadlock on NewWeighted(0). Other methods are untouched, so streaming and initialize/ping are unaffected.
Tests (limits_test.go, in-process client): with max_concurrent_requests=1 two blocking calls never overlap at the backend; request_timeout=20ms unblocks a hung backend call; timed-out calls free their permit; non-positive limit creates no semaphore. Red without the middleware (3 fail), green with it; go test ./pkg/mcp_server/... passes.
This change was prepared with AI assistance (Claude Code) and reviewed before submission.
🤖 Generated with Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…on tool calls
The configured semaphore was created but never acquired and request_timeout
was never applied. Add receiving middleware for tools/call that acquires a
permit (cancellation-aware, released on every exit path) and bounds the call
with the configured deadline. Non-positive values disable the control.
Refs stackql#828
Signed-off-by: Alex Castillo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The timeout does not cancel reverse-proxy database calls, transport-level regression coverage is missing, and the permit-release test can pass despite a semaphore leak.
3 open issues
What changed in this PR
This PR adds shared MCP tool-call admission limits and request deadlines, addressing part of #828. HTTP server timeouts and body limits remain deferred.
Changes:
- Registers middleware that acquires semaphore permits and applies request deadlines to
tools/call.
- Defines non-positive settings as disabling the corresponding control, leaving other methods unchanged.
- Adds in-process tests for concurrency limits, timeouts, permit recovery, and disabled limits.
| File |
Description |
pkg/mcp_server/server.go |
Wires the limits middleware and shared semaphore into server construction. |
pkg/mcp_server/limits.go |
Implements cancellation-aware admission, deadlines, and deferred permit release. |
pkg/mcp_server/limits_test.go |
Adds blocking-backend tests for the configured controls. |
Review effort: Balanced
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
}
|
|
|
defer sem.Release(1)
|
|
|
}
|
|
|
return next(ctx, method, req)
|
|
|
// First call times out and must free its permit; later calls must proceed.
|
|
|
for i := 0; i < 3; i++ {
|
|
|
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
|
|
|
_, _ = cs.CallTool(ctx, &mcp.CallToolParams{Name: "list_providers"})
|
Comment on lines
+301
to
+303
|
|
server.AddReceivingMiddleware(
|
|
|
toolCallLimits(requestSemaphore, time.Duration(config.Server.RequestTimeout)),
|
|
|
)
|
The limits were installed as a receiving middleware. mcp.NewServer installs
the SDK's multi round-trip middleware first, so a middleware added afterwards
wraps it. For clients older than 2026-07-28 the approval prompt is awaited
inside the tools/call, which meant:
- request_timeout counted the time a user took to approve, so a slow
approval failed the call and the approved statement never ran;
- a slot stayed held while a prompt was open, so with a small
max_concurrent_requests one unanswered prompt stalled every other tool
call, server_info included;
- a call refused for lack of a slot never reached the tool gate, so it left
no audit record and surfaced as a protocol error.
Move both limits into a Backend decorator (limitedBackend) applied below the
tool gate. A call that the mode refuses, or that waits on approval, holds no
slot and spends none of its deadline. Limit hits are tool errors that name
the setting and pass through the gate's audit path. A write that times out
warns that it may already have taken effect. validate_select_query no longer
reports a limit hit as valid:false. server_info is not limited.
The reverse proxy backend now passes the context to its database calls, so
the deadline applies there too, and returns row iteration errors instead of
a short result.
Tests: limits_test.go rewritten on testing/synctest (no sleeps), with
end-to-end cases for audited limit errors and for the approval wait sitting
outside the limits. Robot: the startup scenario that set request_timeout to
1ms and expected a live query to succeed now uses 30s (this was the CI
failure); two new stdio scenarios cover the timeout and the concurrency cap.
Docs: request limits section in docs/mcp.md.
Co-Authored-By: Claude Fable 5.2 <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|