GeezerVison

The World Wide Web, restored to 800 pixels of glory.

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

Copy link

Copy Markdown

Contributor

Part of #828: covers the first two bullets of "Required outcome" (bounded admission and request deadlines). The explicit http.Server timeout/body-limit bullet is left for a follow-up, so the issue should stay open.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix(mcp_server): enforce max_concurrent_requests and request_timeout …

cbc9596

…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]>

jeffreyaven requested a balanced review from Copilot

November 7, 2026 23:01

Copilot started reviewing on behalf of jeffreyaven

November 7, 2026 23:02

View session

Copilot AI reviewed

Nov 7, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

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.

All reactions

Comment thread

pkg/mcp_server/limits.go

		}
		defer sem.Release(1)
		}
		return next(ctx, method, req)

Comment thread

pkg/mcp_server/limits_test.go

		// 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 thread

pkg/mcp_server/server.go

Comment on lines +301 to +303

		server.AddReceivingMiddleware(
		toolCallLimits(requestSemaphore, time.Duration(config.Server.RequestTimeout)),
		)

jeffreyaven self-assigned this

Nov 12, 2026

Reviewers

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

jeffreyaven

Labels

None yet

Projects

None yet

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.

3 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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.