Skip to content

mcp: order initialized notification before the first request - #336843

Open
mahsaforati wants to merge 1 commit into
microsoft:mainfrom
mahsaforati:mcp-order-initialized-notification
Open

mahsaforati wants to merge 1 commit into
microsoft:mainfrom
mahsaforati:mcp-order-initialized-notification

Conversation

@mahsaforati

Copy link
Copy Markdown

Fixes #336841

McpHTTPHandle.send() only serializes through _requestSequencer while the transport mode is HttpMode.Unknown. Once initialize returns and the mode becomes HttpMode.Http, later sends take the unserialized path — so notifications/initialized and the first real request go out as two concurrent HTTP POSTs with no ordering guarantee.

When the request wins that race, servers that track session state reject it as uninitialized. The client treats the resulting 400 as a stale session, retries with a new session id, and can lose the race again, so the connection appears permanently broken while succeeding on other attempts. It reproduces more often the higher the latency.

This keeps sends serialized until the initialized notification has actually been delivered, then restores the existing unserialized path. The flag is set only after that POST resolves, so a request entering send() while it is still in flight queues behind it.

Scoped to McpHTTPHandle — no change to IMcpMessageTransport or the extension host protocol, and no added serialization for normal traffic after the handshake.

🤖 Generated with Claude Code

Keep sends serialized until notifications/initialized has been delivered.
Each message is a separate HTTP POST, so otherwise the notification races
the first request that follows it, and servers tracking session state can
reject that request as uninitialized.

Fixes microsoft#336841
Copilot AI balanced review requested due to automatic review settings September 18, 2026 20:59
@vs-code-engineering

Copy link
Copy Markdown
Contributor

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

TylerLeonhardt

Matched files:

  • src/vs/workbench/api/common/extHostMcp.ts

Copilot AI left a comment

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.

🟡 Changes recommended

The sequencer can deadlock when an initialize SSE response remains open, and the timing behavior lacks regression coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Orders MCP initialization traffic to prevent stateful HTTP servers from receiving requests too early.

Changes:

  • Serializes sends until notifications/initialized completes.
  • Detects initialized notifications from JSON-RPC messages.
File summaries
File Description
src/vs/workbench/api/common/extHostMcp.ts Adds initialization tracking and startup request sequencing.
Review details

Suppressed comments (1)

src/vs/workbench/api/common/extHostMcp.ts:394

  • Queued startup sends can stall indefinitely when the initialize response is text/event-stream: _sendStreamableHttp dispatches the JSON-RPC response but awaits _doSSE() until the entire body closes (lines 550–551 and 712–734). If that stream remains open, notifications/initialized is queued behind the still-pending initialize send and is never POSTed, so every subsequent request also blocks. Start the initialized POST once the mode is known, retain its promise, and make only later requests wait for that promise rather than for the initialize response body to close.
			if (this._mode.value === HttpMode.Unknown || !this._didSendInitialized) {
				await this._requestSequencer.queue(async () => {
					await this._send(message);
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +392 to +398
if (this._mode.value === HttpMode.Unknown || !this._didSendInitialized) {
await this._requestSequencer.queue(async () => {
await this._send(message);
if (isInitializedNotification(message)) {
this._didSendInitialized = true;
}
});
@mahsaforati

mahsaforati commented Sep 18, 2026 via email

Copy link
Copy Markdown
Author

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.

MCP: initialized notification is not ordered before the first request, breaking stateful Streamable HTTP servers intermittently

3 participants