chore(mcp): document stateless transport posture and remove SseKeepAliveMiddleware#3501
Conversation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR consolidates OpenTelemetry registration by replacing Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@aspire/Properties/launchSettings.json`:
- Around line 10-11: The launchSettings.json file has misaligned environment
variable values: ASPNETCORE_ENVIRONMENT is set to "dev" while DOTNET_ENVIRONMENT
is set to "Development". Although DOTNET_ENVIRONMENT takes precedence in .NET
10's WebApplicationBuilder, the mismatch creates confusion and should be
resolved for clarity. Change ASPNETCORE_ENVIRONMENT from "dev" to "Development"
to align both variables to the same value, matching the DOTNET_ENVIRONMENT
setting and following Microsoft's recommendations.
In `@src/Elastic.Documentation.ServiceDefaults/AppDefaultsExtensions.cs`:
- Around line 37-40: The AddStandardResilienceHandler() call in
ConfigureHttpClientDefaults applies global resilience policies that retry all
HTTP methods by default, including unsafe operations like POST, PUT, PATCH, and
DELETE which can cause data duplication when retried. Configure the
AddStandardResilienceHandler() method to exclude non-idempotent HTTP methods
from retry logic by passing appropriate configuration options to disable retries
for unsafe methods. Additionally, add the required using statement for
Microsoft.Extensions.Http.Resilience at the top of the AppDefaultsExtensions.cs
file to support the resilience configuration APIs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 49a9b082-7eda-498a-b6c3-1f75398d0c5f
📒 Files selected for processing (17)
Directory.Packages.propsaspire/Properties/launchSettings.jsonsrc/Elastic.Documentation.ServiceDefaults/AppDefaultsExtensions.cssrc/Elastic.Documentation.ServiceDefaults/Extensions.cssrc/Elastic.Documentation.ServiceDefaults/Telemetry/EuidEnrichmentExtensions.cssrc/Elastic.Documentation.ServiceDefaults/Telemetry/OpenTelemetryRegistrationExtensions.cssrc/Elastic.Documentation.ServiceDefaults/Telemetry/VersionHelper.cssrc/api/Elastic.Documentation.Api/OpenTelemetry/OpenTelemetryExtensions.cssrc/api/Elastic.Documentation.Api/Program.cssrc/api/Elastic.Documentation.Mcp.Remote/Program.cssrc/api/Elastic.Documentation.Mcp.Remote/SseKeepAliveMiddleware.cssrc/api/Elastic.Documentation.Mcp.Remote/Telemetry/McpSpanRenameProcessor.cssrc/api/Elastic.Documentation.Mcp.Remote/Telemetry/McpToolTelemetry.cssrc/tooling/docs-builder/DocumentationTooling.cssrc/tooling/docs-builder/Program.cstests-integration/Elastic.Documentation.IntegrationTests/Search/SearchBootstrapFixture.cstests/Elastic.Documentation.Api.Tests/Fixtures/ApiWebApplicationFactory.cs
💤 Files with no reviewable changes (4)
- src/api/Elastic.Documentation.Mcp.Remote/SseKeepAliveMiddleware.cs
- src/Elastic.Documentation.ServiceDefaults/Extensions.cs
- src/Elastic.Documentation.ServiceDefaults/Telemetry/EuidEnrichmentExtensions.cs
- src/tooling/docs-builder/DocumentationTooling.cs
Each MCP tool call now appears as its own transaction in Elastic
Observability instead of collapsing under POST /docs/_mcp/.
- McpSpanRenameProcessor renames the ASP.NET Core server span to
"tools/call {tool}" in OnEnd, after the route-based name is set
- mcp.method.name and mcp.tool.name semconv tags added to server span
- Meter added with mcp.tool.calls counter and mcp.tool.duration
histogram, both dimensioned by tool name, method, profile, outcome
- OTel registration consolidated into AddDocumentationOpenTelemetry
(replaces Extensions.cs and EuidEnrichmentExtensions.cs)
- Aspire bumped to 13.4.3; fixes ExecuteCommandContext.Arguments
required member in integration test bootstrap
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iveMiddleware In SDK 1.4+, stateless mode and legacy SSE are mutually exclusive — enabling both throws at startup. The only mounted route under Stateless=true is POST / (Streamable HTTP); there is no SSE stream to keep alive. Updates the misleading comment that blamed a Cursor bug for the stateless choice: the real reasons are architectural (pure request/response tools, no session affinity at the load balancer). Adds a one-line pointer to mcp-remote for SSE-only clients. Removes SseKeepAliveMiddleware, which allocated an SseKeepAliveStream + SemaphoreSlim on every MCP request despite the keepalive timer never firing in stateless mode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
POST/PUT/DELETE are not idempotent; retrying them can cause duplicate mutations. DisableForUnsafeHttpMethods() restricts the standard resilience retry handler to safe methods only (GET, HEAD, OPTIONS). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
378d9d1 to
b05d62a
Compare
Co-authored-by: Martijn Laarman <Mpdreamz@gmail.com>
Why
The
SseKeepAliveMiddlewarewas added as a workaround for a Cursor SSE-fallback bug, and thestateless-mode comment blamed that same bug. Both are now wrong: in SDK 1.4+, stateless mode and
legacy SSE are mutually exclusive (enabling both throws at startup), and the Cursor bug is an
unfixed client-side regression that server config cannot address.
The keepalive middleware allocated an
SseKeepAliveStream+SemaphoreSlimon every MCPrequest even though no
text/event-streamresponse is ever produced in stateless mode — the timernever started, but the allocations always happened.
What
request/response tools with no server push, running behind a load balancer with no session
affinity. Adds a one-line pointer to the
mcp-remotestdio bridge for SSE-only clients.SseKeepAliveMiddlewareand itsSseKeepAliveStreaminner class (~285 lines).ConfigureHttpJsonOptionscomment to reflect its real purpose (not legacy SSE).How
In ModelContextProtocol.AspNetCore 1.4.0,
WithHttpTransport(o => o.Stateless = true)+MapMcp("")mounts exactly one route:
POST /(Streamable HTTP request/response). Legacy SSE (/sse+/message) is opt-in viaEnableLegacySse(defaultfalse) and throwsInvalidOperationExceptionat startup if combined with
Stateless = true. The SDK emits no SSE heartbeat, but in statelessmode there is no SSE stream to keep alive.