Repository navigation
fix(proxy): give each MCP caller its own upstream session - #160
Merged
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces upstream session management for the MCP proxy (http/sse) to prevent concurrent callers from sharing a single upstream session. It implements the MCP initialize handshake, tracks sessions per session_id, closes idle sessions, and handles session expiration. The review feedback highlights a critical issue where requests might still proceed on a closed proxy when ensureSession returns errProxyClosed; it is recommended to check for this error and abort the request immediately.
blue4209211
marked this pull request as draft
September 30, 2026 03:23
The MCP proxy sent every http/sse request as a bare POST: no initialize handshake, no Mcp-Session-Id, and the caller's session_id was ignored. Concurrent conversations therefore shared one state on stateful MCP servers (browser, shell, database session), and Streamable HTTP servers that require a session could not be used at all. - initialize + notifications/initialized, Mcp-Session-Id on requests, Accept "application/json, text/event-stream" - one upstream session per session_id request param (none = shared) - 30 min idle expiry extended on use; best-effort DELETE on idle eviction and on Close - 404 for a sent session re-initializes and retries once - servers that answer without a session id are remembered as sessionless instead of re-initialized on every call - stdio is unchanged (one process per datasource) and documented so Co-Authored-By: Claude Opus 5.5 <[email protected]>
blue4209211
force-pushed
the
fix/mcp-session-isolation
branch
from
September 30, 2026 03:59
be8493d to
8cedc92
Compare
blue4209211
marked this pull request as ready for review
September 30, 2026 04:00
mayankpande88
approved these changes
Sep 30, 2026
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
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.
Description
Conversations using one MCP server through forager at the same time shared one session, so on stateful servers (browser, shell, database) one saw and changed another's state, with no error. Forager now gives each caller its own MCP session and closes idle ones. MCP servers that require sessions, such as Playwright MCP over HTTP, also start working through forager.
Type of change
How Has This Been Tested?
mcp-proxydatasource at a browser MCP server over HTTP (e.g.npx @playwright/mcp --port 8931).Risks
Checklist
make validatepasses (fmt + lint + test)Engineering detail
Before.
handleHTTPandhandleSSEPOSTed each JSON-RPC message with noinitializehandshake, noMcp-Session-Id, andAcceptset to at most one of the two types. Thesession_idrequest param (the relay forwards the caller'saction_paramsasParams) was ignored.Now (
pkg/proxy/mcp/session.go, http and sse transports):initialize+notifications/initializedpersession_id;Mcp-Session-Idon later requests;Accept: application/json, text/event-stream. SSE-framed replies are unwrapped byContent-Type, or always fortransport: sse, as before.Configureand stopped inClose, DELETEs idle sessions one at a time.CloseDELETEs all live sessions in parallel; a closed proxy opens no new sessions.404for a sent session → re-initialize and retry once (MCP spec). A 400 is never treated as "session gone": guessing from its text would drop a live session and replay a tool call on an ordinary tool error.initializewith no session id, or refuses it with a 4xx, is remembered as sessionless for 30 min, so it isn't re-initialized on every call. Transport errors and 5xx aren't remembered, so a blip can't switch sessions off for a server that needs them.initializerace DELETEs the extra session.Tests (
session_test.go, fake Streamable HTTP server that returns 406 without the dualAccept, 404 for unknown sessions, and SSE-framed replies):session_ids get separate sessions, and onesession_idkeeps its sessioninitializednotification is sentsession_idshares one sessionCloseDELETEs all sessionsmake validate: 0 lint issues, all packages pass under-race.Live check against a real Playwright MCP server (
npx @playwright/mcp@latest --headless --isolated --port 8931). The harness drivesmcp.Proxyexactly as the websocket handler does, against two local pages, PAGE-A and PAGE-B. It is a build-tagged throwaway and is not committed.main(before)conv-a,conv-b) navigate concurrently, then snapshot406 Not Acceptable: Client must accept both application/json and text/event-streamsession_idnavigates to A, then to BCloseends the session upstreamClose, 404 afterSo on
main, forager could not talk to a Streamable HTTP server like Playwright MCP at all.End-to-end through the product (dev environment). This branch build ran as a proxy agent, with a local Playwright MCP datasource. Two chats on the same account ran concurrently: one opened page ALPHA, the other page BRAVO, both waited 15 s, then took a snapshot. From the tool-call records:
BRAVO-4402onlyALPHA-7731onlyWith a shared session, ALPHA's snapshot would have shown BRAVO. Each chat's final answer reported its own marker.
A fresh-context review found and got fixed: the per-call
initializeon sessionless servers, an unsynchronised sweeper start/stop, and the 400-text heuristic.🤖 Generated with Claude Code