[Server] Shield a request handler from foreign fiber suspends - #505
Closed
drubot wants to merge 1 commit into
Closed
[Server] Shield a request handler from foreign fiber suspends#505drubot wants to merge 1 commit into
drubot wants to merge 1 commit into
Conversation
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.
drubot
requested review from
CodeWithKyrian,
Nyholm,
chr-hertel and
soyuka
as code owners
September 10, 2026 19:04
|
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? |
|
Looking at this closer, it seems it can be already done on the caller side, so it seems all alright. Sry for the noise! |
Author
|
Closing: we will handle this on the Drupal side in the mcp_server module instead of changing the SDK's fiber handling. |
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.
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\Fiberand treats every suspension as an MCP protocol yield:Fibers are a process-wide primitive, so any library between
Protocoland 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) andRenderer::executeInRenderContext()(:650) suspend withFiberResumeType::Immediate, whileRegistry::get()(:301) andLocalTaskManager::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:handleFiberYield()logsFiber yielded unexpected payload. payload="Immediate"— once per suspension. A bare\Fiber::suspend()doesn't even get that:BaseTransport::handleFiberYield()returns early onnull, so it strands the handler silently.StreamableHttpTransport::handlePostRequest()sees$this->sessionFiber !== nulland answers the POST withtext/event-streaminstead ofapplication/json. A client that reads only JSON never sees the response, though the handler's writes are already committed.StdioTransport,listen()loops on!feof($input). A client that writes its request and closes stdin leaves the loop beforeprocessFiber()has driven the handler to completion, and the response is dropped outright.An outbound
notificationis lost the same way: whichever suspension comes first wins the single yield slot, so a foreign suspension beforeClientGateway::notify()means the notification is never queued. The third test below covers that.Reproduction
Drupal 11.4.6 +
drupal/mcp_server2.0.0-beta2, onetools/callwhose tool loads entities, posted withAccept: application/json, text/event-stream:Content-Typetext/event-streamapplication/jsonFiber yielded unexpected payloadin the log(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
maintoday.)Fix
Run the handler in its own fiber and drive it from inside the session fiber, so only
FiberSuspendpayloads reach the transport:ClientGateway'snotification/requestpayloads 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, theInputRequiredShimstill re-enters the handler inside one fiber, andhandleFiberYield()'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::Immediateis precisely a request for immediate resumption, andRenderer::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|Errorornullmeant 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 onmainand passing with the patch:'Immediate', thennull) is driven to completion in band, andattachFiberToSession()is never called;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_serverunder Drupal at drunomics; the drupal.org side is mcp_server#3585917. Drafted with Claude Code from a drunomics dev VM.