fix(sdk): forward hasPerRequestStream so modern cancellation aborts t… - #4474
fix(sdk): forward hasPerRequestStream so modern cancellation aborts t…#4474ignaciojimenezr wants to merge 1 commit into
Conversation
…he stream MCP 2026-07-28 makes closing the per-request stream the cancellation signal on Streamable HTTP, and says no notifications/cancelled is expected. The client package forks on `era === modern && transport.hasPerRequestStream === true`, but every transport we pass to a client is wrapped, and the wrappers forwarded sessionId and setProtocolVersion while dropping hasPerRequestStream. The flag read undefined, so the fork took the legacy branch on every connection — including plain ones, since wrapTransportForTaskResults is applied unconditionally at each HTTP transport site. We POSTed a notification modern servers do not expect, and because that notification is a separate request from the call being cancelled, the server ran the tool to completion having never seen the one signal that would have reached it. Forward the flag from all five wrappers (four in transport-utils, one in the widget runtime). Forwarded, not defaulted: over stdio the notification is still the correct signal. The existing modern-abort test passed the whole time because the legacy branch POSTs fire-and-forget and the assertion ran before it landed. Let that settle, and also assert the aborted tools/call exchange actually terminates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
dosubot
Bot
added
size:L
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. WalkthroughTransport wrappers now forward Merge Risk: ⚪ Minimal · up to This change makes modern HTTP cancellation terminate the active request stream while preserving legacy behavior for transports without per-request streams. No actionable merge-blocking risk remains beyond normal checks and review. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
…he stream
MCP 2026-07-28 makes closing the per-request stream the cancellation signal on Streamable HTTP, and says no notifications/cancelled is expected. The client package forks on
era === modern && transport.hasPerRequestStream === true, but every transport we pass to a client is wrapped, and the wrappers forwarded sessionId and setProtocolVersion while dropping hasPerRequestStream.The flag read undefined, so the fork took the legacy branch on every connection — including plain ones, since wrapTransportForTaskResults is applied unconditionally at each HTTP transport site. We POSTed a notification modern servers do not expect, and because that notification is a separate request from the call being cancelled, the server ran the tool to completion having never seen the one signal that would have reached it.
Forward the flag from all five wrappers (four in transport-utils, one in the widget runtime). Forwarded, not defaulted: over stdio the notification is still the correct signal.
The existing modern-abort test passed the whole time because the legacy branch POSTs fire-and-forget and the assertion ran before it landed. Let that settle, and also assert the aborted tools/call exchange actually terminates.
Summary by cubic
Fixes modern MCP cancellation by forwarding
hasPerRequestStreamthrough all transport wrappers, so the protocol layer aborts the per-request stream instead of POSTingnotifications/cancelled. Previously the flag was dropped, so every connection took the legacy branch, sending a signal modern servers neither expect nor receive.transport-utilsand the widget runtime'sLoggingTransport.tools/callexchange actually terminates.Written for commit 246472d. Summary will update on new commits.