Skip to content

[integrations] Fix per-request McpServer instantiation for stable MCP connections#261

Merged
justfinethanku merged 4 commits into
NateBJones-Projects:mainfrom
yuens1002:contrib/yuens1002/per-request-mcp
Jun 12, 2026
Merged

[integrations] Fix per-request McpServer instantiation for stable MCP connections#261
justfinethanku merged 4 commits into
NateBJones-Projects:mainfrom
yuens1002:contrib/yuens1002/per-request-mcp

Conversation

@yuens1002

Copy link
Copy Markdown
Contributor

Contribution Type

  • Repo improvement (docs, CI, templates)

What does this do?

Both server/index.ts and integrations/kubernetes-deployment/index.ts instantiate a global McpServer singleton at module load and call server.connect(transport) on every request — attaching a new transport to an already-connected server on each hit. Under concurrent load or repeated calls this corrupts transport state and produces the reconnect instability that surfaces on claude.ai.

This PR wraps all tool registrations in a buildServer() factory and calls it per-request, so each request gets a fresh McpServer + transport pair with no shared state. It also strips mcp-session-id from responses (the server is stateless — advertising a session ID misleads clients into expecting resumption that never comes), awaits the transport response for proper post-processing, and adds a null-guard with a 500 fallback.

Additionally backfilled into integrations/kubernetes-deployment/index.ts, which was missing them entirely: CORS preflight handler, Accept header patch for Claude Desktop connector compatibility (already in server/index.ts since #33), and CORS headers on 401 and success responses.

A self-contained test (server/test-stateless.mjs) is included. It requires no Supabase instance — MCP initialize is a pure protocol handshake that never touches the database.

Requirements

No new external services. Works with any existing OB1 setup. To run the test:

cd server && npm install
node test-stateless.mjs
# 20 assertions: 20 passed, 0 failed

Checklist

  • I've read CONTRIBUTING.md
  • I tested this on my own Open Brain instance
  • No credentials, API keys, or secrets are included
  • Automated test passes: 20/20 assertions, no infra required

Context for reviewers: This fix was developed by debugging reconnect instability in a production OB1 deployment. The singleton bug was the confirmed root cause after 6 weeks of investigation. The per-request buildServer() pattern has been stable in that deployment since landing. This PR ports it back upstream so the canonical server carries the same fix.

Note on automated title check: The changes span server/index.ts and integrations/kubernetes-deployment/index.ts[integrations] is the closest valid prefix for the automated check. The core server fix is the same pattern applied to both files. Requesting a human override if the scope check flags server/index.ts.

yuens1002 added 2 commits May 4, 2026 15:46
… connections

Both server/index.ts and integrations/kubernetes-deployment/index.ts shared
a global McpServer singleton and called server.connect(transport) on every
request, attaching new transports to an already-connected server. Under
concurrent load or repeated calls this causes transport state corruption and
the reconnect instability reported on claude.ai.

Fix: wrap all tool registrations in a buildServer() factory function and call
it per-request, so each request gets a fresh McpServer + transport pair with
no shared state.

Also applied to both files:
- Strip mcp-session-id from responses: server is stateless and advertising a
  session ID misleads clients into expecting resumption that never comes
- Await transport.handleRequest() to enable response post-processing
- Null-guard on the transport response with a 500 fallback

Additionally backfilled into integrations/kubernetes-deployment/index.ts:
- CORS preflight handler (OPTIONS *) — was missing entirely
- Accept header patch for Claude Desktop connector compatibility
  (mirrors the fix already present in server/index.ts since issue NateBJones-Projects#33)
- CORS headers on 401 responses and successful MCP responses
Validates the per-request McpServer pattern without any Supabase instance.
MCP initialize is a pure protocol handshake — no tools are called, no DB touched.

Tests (20/20):
- CORS preflight: OPTIONS → 200 with correct headers
- Auth: wrong/missing key → 401 with CORS headers
- MCP initialize: 200, no mcp-session-id, CORS on success, valid protocolVersion
- Per-request isolation: two sequential initializes both succeed with no
  mcp-session-id, proving the singleton is gone
- tools/list: server responds cleanly, correct headers

Setup (from server/ directory):
  npm install
  node test-stateless.mjs   # or: npm test
Copilot AI review requested due to automatic review settings May 4, 2026 19:48
@github-actions github-actions Bot added the integration Contribution: MCP extension or capture source label May 4, 2026
@github-actions

github-actions Bot commented May 4, 2026

Copy link
Copy Markdown

Hey @yuens1002 — welcome to Open Brain Source! 👋

Thanks for submitting your first PR. The automated review will run shortly and check things like metadata, folder structure, and README completeness. If anything needs fixing, the review comment will tell you exactly what.

Once the automated checks pass, a human admin will review for quality and clarity. Expect a response within a few days.

If you have questions, check out CONTRIBUTING.md or open an issue.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes MCP connection instability by eliminating the module-level McpServer singleton and instead creating a fresh McpServer + StreamableHTTPTransport per request, preventing multiple transports from being attached to a single long-lived server instance.

Changes:

  • Wrapped tool registrations in a buildServer() factory and instantiated the server per request in both the canonical server and the Kubernetes deployment variant.
  • Awaited transport.handleRequest, added a null-response guard, stripped mcp-session-id, and ensured CORS headers are present on success and error responses.
  • Added a self-contained Node-based statelessness test harness under server/ with an accompanying package.json.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
server/index.ts Refactors MCP server creation to be per-request and post-processes transport responses (CORS + header stripping + null guard).
integrations/kubernetes-deployment/index.ts Applies the same per-request server pattern and backfills CORS + Accept-header patch behavior.
server/test-stateless.mjs Adds a Node script to validate per-request isolation and stateless response headers without external infra.
server/package.json Adds a minimal Node test harness configuration for running test-stateless.mjs.
Comments suppressed due to low confidence (2)

server/index.ts:105

  • Inside buildServer(), the function body isn’t indented (e.g., const server = ... starts at column 0). This makes it easy to misread what is inside the factory vs. module scope and is inconsistent with the surrounding indentation in this file; please indent the buildServer() body consistently.
function buildServer(): McpServer {
const server = new McpServer({
  name: "open-brain",
  version: "1.0.0",
});

integrations/kubernetes-deployment/index.ts:144

  • buildServer()’s body is not indented (the const server = ... line and subsequent tool registrations are aligned with top-level code). Please indent the factory body to match the rest of the file so it’s clear which code executes per-request vs. at module load.
function buildServer(): McpServer {
const server = new McpServer({
  name: "open-brain",
  version: "1.0.0",
});

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/package.json
Address Copilot review: factory function body was at column 0, making it
ambiguous what runs per-request vs. at module load. Indent all content
inside buildServer() by two spaces in both server/index.ts and
integrations/kubernetes-deployment/index.ts.
@yuens1002

yuens1002 commented May 4, 2026

Copy link
Copy Markdown
Contributor Author

.github/workflows/ob1-gate.yml
Invalid workflow file
(Line: 52, Col: 14): Exceeded max expression length 21000

The ob1-gate workflow failure is a pre-existing YAML expression length issue (the run block exceeds GitHub's 21,000-char limit). This PR didn't touch any workflow files.

@alanshurafa alanshurafa added area: integrations Review area: integrations/MCP/capture sources review: ready-for-maintainer Community reviewer recommends maintainer review alan-reviewed Reviewed by Alan Shurafa in Community Reviewer role labels May 20, 2026
@alanshurafa

Copy link
Copy Markdown
Collaborator

Thanks for the contribution, and welcome. The diagnosis here is correct and the bug is real — a module-load McpServer singleton with a per-request server.connect(transport) does corrupt transport state under concurrent load, and the reconnect instability on claude.ai is a known symptom. Moving tool registration into a per-request buildServer() factory so each request gets a fresh server and transport is the right fix, and the test-stateless.mjs test is a good addition.

Two things for the maintainer. This is a large rewrite of server/index.ts (the +980/-743 is mostly the re-wrap), and it touches the same file as #238 (OAuth) and #243 (auth envelopes) — so merge ordering across those three needs coordinating. Separately, your note about the ob1-gate.yml expression-length error is a useful catch — that is a real pre-existing gate problem, not caused by this PR, and it lines up with the gate issues #308 is addressing. Recommend maintainer review.

— Alan (community reviewer; non-binding)

@justfinethanku justfinethanku left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated the refreshed branch after updating from main. Reviewed the current per-request server factory diff, confirmed #243 is already merged, and ran the bundled stateless transport test locally on the refreshed head (20/20 assertions passing). Approving for maintainer merge; remaining blocked checks are repository-policy mechanics rather than an unresolved code issue on this branch.

@justfinethanku justfinethanku merged commit 622ab69 into NateBJones-Projects:main Jun 12, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

alan-reviewed Reviewed by Alan Shurafa in Community Reviewer role area: integrations Review area: integrations/MCP/capture sources integration Contribution: MCP extension or capture source review: ready-for-maintainer Community reviewer recommends maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants