Copilot commented on code in PR #13482:
URL: https://github.com/apache/apisix/pull/13482#discussion_r3411241585
##########
apisix/plugins/mcp/server_wrapper.lua:
##########
@@ -50,10 +67,45 @@ local function sse_handler(conf, ctx, opts)
end
server:start() -- this is a sync call that only returns when the
client disconnects
end
+end
+
+
+local function sse_handler(conf, ctx, opts)
+ local server = opts.server
- if opts.event_handler.on_disconnect then
- opts.event_handler.on_disconnect({ server = server })
+ -- bound the number of concurrent sessions a worker will keep open, so a
+ -- route cannot be driven to spawn an unbounded number of backend processes
+ local max_sessions = conf.max_sessions or DEFAULT_MAX_SESSIONS
+ if not session_limit.acquire(max_sessions) then
+ core.log.warn("mcp session limit reached (", max_sessions,
+ "), rejecting new SSE connection")
+ return core.response.exit(429)
Review Comment:
When the session ceiling is reached this returns a bare 429 with no response
body, which makes it hard for clients/operators to understand why the SSE
connection was rejected. Consider returning a small JSON body with a message
(consistent with other plugin error responses).
##########
apisix/plugins/mcp/session_limit.lua:
##########
@@ -0,0 +1,50 @@
+--
+-- Licensed to the Apache Software Foundation (ASF) under one or more
+-- contributor license agreements. See the NOTICE file distributed with
+-- this work for additional information regarding copyright ownership.
+-- The ASF licenses this file to You under the Apache License, Version 2.0
+-- (the "License"); you may not use this file except in compliance with
+-- the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+--
+
+-- Tracks the number of live MCP SSE sessions handled by the current worker so
+-- that a single route cannot spawn an unbounded number of backend processes.
+local _M = {}
+
+
+local count = 0
+
+
+-- Try to reserve a slot for a new session. Returns true if the worker is below
+-- the configured ceiling, false otherwise. Every successful acquire() must be
+-- balanced by exactly one release().
+function _M.acquire(max)
+ if count >= max then
+ return false
+ end
+ count = count + 1
+ return true
+end
Review Comment:
`session_limit.acquire(max)` will throw a runtime error if `max` is nil or
non-numeric (e.g., misconfiguration or future call sites), because it does
`count >= max` without validation. Since this module is meant to be reused and
unit-tested, it should defensively handle invalid `max` values and simply
reject the acquire.
##########
t/plugin/mcp-bridge.t:
##########
@@ -57,3 +57,62 @@ property "command" is required
property "command" validation failed: wrong type: expected string, got number
done
property "args" validation failed: wrong type: expected array, got string
+
+
+
+=== TEST 2: max_sessions schema validation
+--- config
+ location /t {
+ content_by_lua_block {
+ local test_cases = {
Review Comment:
The new `max_sessions` behavior is only covered by schema validation and
unit-level acquire/release checks, but there is no integration test asserting
that the SSE endpoint actually returns 429 when the ceiling is exceeded (and
that a disconnect frees a slot). Given the operational impact (process
spawning), it would be good to add an end-to-end test for the handler behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
