Skip to content

[Server] Shield a request handler from foreign fiber suspends - #505

Closed
drubot wants to merge 1 commit into
modelcontextprotocol:mainfrom
drubot:fiber-shield-foreign-suspends
Closed

[Server] Shield a request handler from foreign fiber suspends#505
drubot wants to merge 1 commit into
modelcontextprotocol:mainfrom
drubot:fiber-shield-foreign-suspends

Conversation

@drubot

@drubot drubot commented Sep 10, 2026

Copy link
Copy Markdown

While working on the Drupal MCP server, we ran into the following issue: https://git.drupalcode.org/project/mcp_server/-/work_items/3585917 - This PR tries to address this.

NOTE: This is a coding agent generated PR via Claude code.


A request handler is stranded when the host framework suspends the fiber

Protocol::handleRequest() runs each handler inside a \Fiber and treats every suspension as an MCP protocol yield:

$fiber = new \Fiber(static function () use ($handler, $request, $session, $shim, $codec): Response|Error { … });
$result = $fiber->start();

if ($fiber->isSuspended()) {
    if (\is_array($result) && isset($result['type'])) { /* notification | request */ }
    $transport->attachFiberToSession($fiber, $session->getId());

    return;
}

Fibers are a process-wide primitive, so any library between Protocol and the handler may suspend the running fiber for its own scheduling. Drupal core does exactly that, to batch slow lookups rather than to await anything — on 11.4.6, EntityStorageBase::loadMultiple() (:313), AliasManager::getAliasByPath() (:141) and Renderer::executeInRenderContext() (:650) suspend with FiberResumeType::Immediate, while Registry::get() (:301) and LocalTaskManager::getLocalTasks() (:363) suspend with nothing at all. None of those values carries protocol meaning; the handler is simply asking to be resumed.

The SDK takes each of them for a yield. The is_array($result) && isset($result['type']) guard means nothing is sent, and the fiber is handed to the transport with no message attached, so:

  1. handleFiberYield() logs Fiber yielded unexpected payload. payload="Immediate" — once per suspension. A bare \Fiber::suspend() doesn't even get that: BaseTransport::handleFiberYield() returns early on null, so it strands the handler silently.
  2. StreamableHttpTransport::handlePostRequest() sees $this->sessionFiber !== null and answers the POST with text/event-stream instead of application/json. A client that reads only JSON never sees the response, though the handler's writes are already committed.
  3. On StdioTransport, listen() loops on !feof($input). A client that writes its request and closes stdin leaves the loop before processFiber() has driven the handler to completion, and the response is dropped outright.

An outbound notification is lost the same way: whichever suspension comes first wins the single yield slot, so a foreign suspension before ClientGateway::notify() means the notification is never queued. The third test below covers that.

Reproduction

Drupal 11.4.6 + drupal/mcp_server 2.0.0-beta2, one tools/call whose tool loads entities, posted with Accept: application/json, text/event-stream:

v0.7.1 with this patch
response Content-Type text/event-stream application/json
Fiber yielded unexpected payload in the log 11 0

(That measurement is from the Drupal side, on the release we run; the three unit tests added here are the in-repo evidence, and each fails on main today.)

Fix

Run the handler in its own fiber and drive it from inside the session fiber, so only FiberSuspend payloads reach the transport:

private static function runShielded(callable $exchange): Response|Error
{
    $fiber = new \Fiber($exchange);
    $yielded = $fiber->start();

    while (!$fiber->isTerminated()) {
        if (\is_array($yielded) && isset($yielded['type'])) {
            $yielded = $fiber->resume(\Fiber::suspend($yielded));

            continue;
        }

        $yielded = $fiber->resume();
    }

    return $fiber->getReturn();
}

ClientGateway's notification / request payloads pass through unchanged — the shield re-yields them to the session fiber and hands the peer's answer back to the handler — so elicitation and sampling keep their current semantics, the InputRequiredShim still re-enters the handler inside one fiber, and handleFiberYield()'s warning stays the guard it was meant to be. No transport changes.

The judgement call, and what it does not fix

A foreign suspension is resumed on the spot, because the SDK has no scheduler and there is no other fiber to run. That is right for a host that suspends to group work — Drupal's FiberResumeType::Immediate is precisely a request for immediate resumption, and Renderer::executeInRenderContext() drives its own fibers with the same loop.

It is not a general async runtime. A handler that suspends to await I/O — say through an Amp-backed client — gets resumed before its future settles. Today that case is broken too, and worse: the transport resumes it later with a Response|Error or null meant for the SDK's own wait, which is #504 on the client side of the same assumption. So this is not a regression for that host, but it is not the answer for it either; a scheduler-aware SDK is. I'd rather have that stated in review than assumed.

Tests

Three cases in tests/Unit/Server/ProtocolTest.php, all failing on main and passing with the patch:

  • a handler suspended by the host ('Immediate', then null) is driven to completion in band, and attachFiberToSession() is never called;
  • an outbound notification still reaches the transport when a foreign suspension precedes it;
  • an outbound request still receives the peer's answer, with foreign suspensions on both sides of the round trip.

make cs (0 files changed), make phpstan (no errors), unit 1583, integration 68 and inspector 103/7 skipped are green locally on PHP 8.5.4.


Reported and fixed while running mcp_server under Drupal at drunomics; the drupal.org side is mcp_server#3585917. Drafted with Claude Code from a drunomics dev VM.

Fibers are process-wide, so a host framework may suspend the running fiber
between Protocol and the handler for its own scheduling. Protocol read every
suspension as an MCP yield, stranding the handler with no message attached.
Drive the handler from an inner fiber so only ClientGateway payloads reach the
transport.
@fago

fago commented Sep 11, 2026

Copy link
Copy Markdown

Note: The issue fix and PR is opened by our coding agent powered by claude code.

I can confirm it fixes the issue in Drupal well, but honestly I'm not sure this is the right way to go. As described in the section "The judgement call, and what it does not fix", it's not the right fix for every caller. So we might want to find a more flexible solution here. What about making ::runShielded() protected so an implementing framework could customize it?

@fago

fago commented Sep 11, 2026

Copy link
Copy Markdown

Looking at this closer, it seems it can be already done on the caller side, so it seems all alright. Sry for the noise!

@drubot

drubot commented Sep 11, 2026

Copy link
Copy Markdown
Author

Closing: we will handle this on the Drupal side in the mcp_server module instead of changing the SDK's fiber handling.

@drubot drubot closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants