Copilot commented on code in PR #13482:
URL: https://github.com/apache/apisix/pull/13482#discussion_r3410954661
##########
apisix/plugins/mcp/server_wrapper.lua:
##########
@@ -50,10 +67,38 @@ 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
+
+ -- 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)
+ end
- if opts.event_handler.on_disconnect then
+ local ok, code, body = pcall(run_session, conf, opts, server)
+
+ -- always release: tear down the backend process/broker state and free the
+ -- session slot regardless of how the loop ended
+ if opts.event_handler and opts.event_handler.on_disconnect then
opts.event_handler.on_disconnect({ server = server })
- server:close()
+ end
+ server:close()
+ session_limit.release()
Review Comment:
Cleanup (on_disconnect/server:close/session_limit.release) isn't protected.
If a user-supplied on_disconnect handler (or close) throws, the session slot
won't be released and backend/broker teardown may be skipped, potentially
locking the worker at the max_sessions ceiling until reload. Guard cleanup with
pcall and ensure session_limit.release() always runs.
--
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]
