Skip to content

server : add experimental MCP (stdio) server support - #25736

Closed
pwilkin wants to merge 14 commits into
ggml-org:masterfrom
pwilkin:feat-mcp-server
Closed

server : add experimental MCP (stdio) server support#25736
pwilkin wants to merge 14 commits into
ggml-org:masterfrom
pwilkin:feat-mcp-server

Conversation

@pwilkin

@pwilkin pwilkin commented Jul 15, 2026

Copy link
Copy Markdown
Member

Overview

Adds support for connecting llama-server to external MCP (Model Context Protocol) servers over stdio, exposing their tools through the existing /tools endpoint alongside the built-in ones.

Servers are configured in the Cursor-compatible JSON format, via either --mcp-servers-config <path> or --mcp-servers-json <inline>. At startup each configured server is spawned once to discover its tools, then shut down; instances are spawned lazily on first tool call and kept for the lifetime of the server process.

Additional information

Important limitation for now: MCP instances are global rather than per-slot. The /tools endpoint has no slot binding, so keying instances by slot would tie external process lifetime to an unrelated inference slot's release, tearing down stateful MCP servers on every completion.

The feature is off by default and marked experimental: enabling it lets the server spawn arbitrary local processes, so it should not be used in untrusted environments.

Tool names are exposed as _ and skipped on collision with an existing tool, so an MCP server cannot shadow a built-in one.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: Yes, an adversarial framework with a coding council of StepFun 3.7 + DS4Pro as coders + Claude Opus 4.8 as the reviewer, based on my specs.

@pwilkin
pwilkin requested review from a team as code owners July 15, 2026 20:04
@allozaur

Copy link
Copy Markdown
Contributor

Well, I think that this is also strongly related to #20769

In general, maybe it's a good moment to have the implementation completed to support all connection types (Streamable HTTP, SSE (legacy), WebSocket, stdio) + configuration via mcp.json that is integrated with the llama-ui /mcp-servers route?

cc @ngxson @ServeurpersoCom

@pwilkin

pwilkin commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

@allozaur definitely, but I think unifying the two is better left as a follow-up PR

@ggerganov

Copy link
Copy Markdown
Member

What is the advantage of this compared to adding the MCP servers in the usual way?

@pwilkin

pwilkin commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

@ggerganov this allows using STDIO (spawned process) MCP servers - which is still the majority of MCP servers available. The previous method only allowed connecting to already running MCP servers via HTTP(S).

@ServeurpersoCom

ServeurpersoCom commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Tools become usable by any API client (agents, scripts, curl), not just an open WebUI session. Before merging this needs massive testing on edge cases and cross-platform, Windows especially (stderr piped into stdout, arg quoting).

For example, in my case:
Right now, to test the built-in tools, I have to run llama-server itself inside the same container for security reasons.
With this PR llama-server can run in its own container and pipe tool commands into a separate execution container. That is what I am currently doing with my mcp.js server, just over MCP/HTTP instead of stdio.

In the end, this PR brings the modularity needed for advanced users to pipe tool calls into an MCP client wrapper or anything else, effectively enabling backend-side MCP like the Anthropic and OpenAI cloud website. One can even imagine backend-side home automation working with any frontend.

@pwilkin

pwilkin commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

Yeah, also stuff like integrating with running instances (Godot MCP, Blender MCP etc.)

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

I'm going to test it out tonight when I get home from work.

@ServeurpersoCom

ServeurpersoCom commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Each new MCP server accidentally inherited the pipes of the previous ones, which could prevent clean shutdowns and accumulate unnecessary file descriptors :

Small fix:
39ea182

Linux look good with this.

Now I run Windows tests...
cmd run + powershell run + powershell MCP server

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

Windows MCP processes received corrupted path arguments, and their stderr output could corrupt the JSON-RPC stream.

Fix:
a9d5a08

@pwilkin

pwilkin commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

@ServeurpersoCom can you please propose that as a PR on my branch?

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

@ServeurpersoCom can you please propose that as a PR on my branch?

Yes.

What I tested :

Linux - Tested

  • MCP stdio handshake, tool discovery, and tool calls
  • Persistent child process and per-process state
  • Crash detection, automatic respawn, and state reset
  • Concurrent calls with no response mixing
  • Multiple MCP servers running independently
  • File descriptor inheritance reproduced via /proc
  • Pipe leakage reproduced again after child respawn
  • Post-fix descriptor isolation and functional regression tests

Linux - Fixed

  • Added FD_CLOEXEC to all MCP pipe endpoints
  • Prevented children from inheriting other MCP instances' pipes
  • Added safe fcntl retry and cleanup handling
  • Kept the implementation portable across POSIX platforms

Windows - Tested

  • MCP stdio using native PowerShell child processes
  • Multiple persistent and independent MCP servers
  • Concurrent calls and cross-server non-blocking behavior
  • High-concurrency stress with exact result validation
  • Paths containing spaces and backslashes
  • Exact command-line argument preservation
  • Complete and unterminated stderr output
  • JSON-RPC corruption caused by stderr/stdout mixing
  • Full post-fix regression and stress testing

Windows - Fixed

  • Replaced broken Windows argument quoting with CRT-compatible quoting
  • Preserved spaces, quotes, trailing backslashes, and empty arguments
  • Separated child stderr from the JSON-RPC stdout pipe
  • Restored reliable warmup and tool calls for verbose MCP servers
  • Re-run Linux with Windows patch.

@pwilkin

pwilkin commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

Thanks, my Windows dev environment isn't really set up :)

@ServeurpersoCom

ServeurpersoCom commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

We can do a ton of great things with it, and it's cross-platform. You can even create a simple wrapper in any language to re-output from the backend via Streamable-HTTP, SSE, or WebSockets with custom security features.

We can use poll.h instead of sys/select.h here: select() silently breaks when stdout_fd exceeds FD_SETSIZE (1024), which can happen on a server holding many connections, while poll() has no fd limit.

@allozaur
allozaur requested a review from ngxson July 20, 2026 14:29
@allozaur

Copy link
Copy Markdown
Contributor

@ngxson wdyt about this?

"initialize": handle_initialize,
"tools/list": handle_tools_list,
"tools/call": handle_tools_call,
"shutdown": handle_shutdown,

@chansikpark chansikpark Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interested in this PR and took a glance. Was curious to see how a minimal MCP server was implemented as I just worked through the same.

The method "shutdown", and the associated handler, appear to be confabulated. There's no such method in the MCP schema, and the behaviour actually resembles the ping method. Couldn't find it used in the test runner either.

It's test code, so this is a nit, but seemed a bit worrisome. I haven't read much of the PR at all, so please excuse me if I'm off basis here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is indeed a pattern reproduction; the fix is:

diff --git a/tools/server/tests/fixtures/mcp_echo_server.py b/tools/server/tests/fixtures/mcp_echo_server.py
index 6969f06ec..7acfb3588 100755
--- a/tools/server/tests/fixtures/mcp_echo_server.py
+++ b/tools/server/tests/fixtures/mcp_echo_server.py
@@ -108,10 +108,10 @@ def handle_tools_call(params, req_id):
         return {
             "jsonrpc": "2.0",
             "id": req_id,
-            "error": {"code": -32601, "message": f"Unknown tool: {tool_name}"}
+            "error": {"code": -32602, "message": f"Unknown tool: {tool_name}"}
         }

-def handle_shutdown(params, req_id):
+def handle_ping(params, req_id):
     return {
         "jsonrpc": "2.0",
         "id": req_id,
@@ -122,7 +122,7 @@ HANDLERS = {
     "initialize": handle_initialize,
     "tools/list": handle_tools_list,
     "tools/call": handle_tools_call,
-    "shutdown": handle_shutdown,
+    "ping": handle_ping,
 }

 def main():
@@ -143,6 +143,10 @@ def main():
         req_id = request.get("id")
         params = request.get("params", {})

+        # JSON-RPC 2.0: a message without an id is a notification and must not receive a response
+        if req_id is None:
+            continue
+
         handler = HANDLERS.get(method)
         if handler:
             response = handler(params, req_id)

@ngxson ngxson 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.

IMO the server code is quite messy & over-complicated. I don't get why we need to reinvent the whole process management system, while we already had subprocess.h

I will try vibe-coding my way to see if it's better. if it is, I'll replace this PR with mine

a general note on features like this in the future: if you plan to vibe code it but you only know 50% of the full picture, please, just open an issue. subprocess and multi-thread management is not something safe to vibe without a strong knowledge about OS designs. even claude makes mistake about thread-safety sometimes

Comment thread tools/server/server-mcp.h Outdated
std::string error;
uint64_t next_id = 1;
int timeout_ms = 30000;
mutable std::recursive_mutex mutex;

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.

I'm a simple man: I see mutable mutex without an explanation, I know this code is not good

Comment thread tools/server/server-mcp.h Outdated

// shared with every spawned instance so begin_shutdown() can signal them all
// with one lock-free store, without iterating global_instances
std::shared_ptr<std::atomic<bool>> shutting_down = std::make_shared<std::atomic<bool>>(false);

@ngxson ngxson Jul 22, 2026

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.

maybe I get it wrong but it feels like this is a hack to address the problem where server_mcp_instance might be deleted before begin_shutdown() finishes; if it's true, that's a bad design.

@ServeurpersoCom

ServeurpersoCom commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Just to make sure I follow: subprocess.h is already vendored and the PR reimplements all of that from scratch, which would explain why we ended up fixing the quoting and pipe inheritance by hand.
The feature itself is worth having though, so getting a clean version of it in would be great.

@pwilkin

pwilkin commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

Eh. I'll rewrite it by hand. Was hoping to get it done by bruteforcing CC, but obviously that just makes a bigger mess.

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

With the right prompt, and by feeding it all the examples and reviews of ngxson and his style it can manage to produce something almost perfect that is easy to review!

@pwilkin

pwilkin commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

In this case, just telling it "this project has subprocess.h, use it" would probably help a lot. But it's just another proof I'm getting lazy and not reviewing the code sufficiently, so I'll do it by hand :P

@ngxson

ngxson commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

it's not as simple as that, claude or any claude-distilled models tend to make excessive use of shared_ptr, mutex and atomics and combination of them when comes to thread-safety. that works but is the bruteforce solution, it's quite messy, especially when tracking the object's file cycle.

to be clear, I don't mean we shouldn't use AI at all, but it is just not good at designing thread-safe code. it is still good at writing and reviewing it.

@pwilkin

pwilkin commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

@ngxson the initial design was OK, but then CC's initial self-review caught some bugs, which it decided to patch up with the mutexes, which revealed more bugs... and so on :)

That's why I have to rewrite it by hand.

@ngxson ngxson mentioned this pull request Jul 24, 2026
4 tasks
@pwilkin
pwilkin force-pushed the feat-mcp-server branch 2 times, most recently from d50800e to 8338c74 Compare July 24, 2026 10:19
@pwilkin
pwilkin requested a review from ggerganov as a code owner July 24, 2026 10:19
pwilkin added 2 commits July 24, 2026 17:12
Builds on the transport/manager architecture (server_mcp_transport + server_pipe)
with the hardening and integration the draft did not yet have.

Hardening:
* Reader and stderr pumps are polled (running-aware) instead of blocking on a read
  that only ends at EOF. subprocess_terminate() SIGKILLs only the direct child, so a
  grandchild the MCP server spawned that inherited the pipe would otherwise keep the
  write end open and hang teardown (both warmup shutdown at startup and process
  shutdown). The writer is likewise non-blocking + polled.
* Windows: resolve the command through PATHEXT so "npx" (npm ships npx.cmd, never
  npx.exe) spawns, matching POSIX's PATH search; and enumerate the parent environment
  as UTF-8 (GetEnvironmentStringsW) instead of the active code page.
* server_pipe gains an opt-in max_size (default unbounded, so the router's streaming
  use is unchanged); the MCP reply queue uses it so a server that streams unsolicited
  notifications between requests cannot grow it without bound.

Integration:
* --mcp-servers-config / --mcp-servers-json flags; enabling MCP restricts default CORS
  to localhost, same as --tools.
* MCP tools are exposed through /tools (and chat-completions) as <server>_<tool>,
  skipping names that collide with a built-in or another MCP tool.
* Manager lifecycle wired into llama_server(): warmup at start, shutdown() from the
  signal handler before the HTTP server drains, blocking teardown in clean_up().
* SIGPIPE ignored so a child dying mid-write yields EPIPE rather than killing us.

Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
21 tests over the /tools endpoint: tool discovery/invocation, timeouts, crash
recovery and respawn cooldown, warmup partial failure, malformed and batched
notification+response output, tool-definition shape, and prompt shutdown during a
slow call.

The last test spawns an MCP server that leaves a grandchild inheriting its
stdout/stderr and asserts the server both starts and stops promptly. Verified it
fails (5s SIGKILL fallback on a deadlocked reader-join) when the pump is made to
ignore the running flag, and passes with the polled reader.

Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
@pwilkin

pwilkin commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

Superseded by #26062

@pwilkin pwilkin closed this Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants