StreamableHTTPServerTransport should only check init status when sessionId existing

[qikaigao]

Motivation and Context

When using stateless mode, the backend will create a new transport for each request. For new transport, the initialized filed is always false. So we should only check the initialized filed if there is session existing

How Has This Been Tested?

Tested in local MCP Server. At backend, I create a new transport object and meet the issue. the PR fixed it.

Breaking Changes

No

Types of changes

• [ X] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• [X ] I have read the MCP Documentation
• [ X] My code follows the repository's style guidelines
• [ X] New and existing tests pass locally
• [ X] I have added appropriate error handling
• [ X] I have added or updated documentation as needed

Additional context

[cliffhall]

A more understandable alternative would be:

1. Remove this change
2. Move the subsequent conditional at lines 478..482 up before this condition.

This would handle the stateless condition out of the gate, and the reader would automatically understand that the rest of the method is dealing with stateful sessions.

[qikaigao]

Yes, I agree with the change. Thank you for point it out!

[cliffhall]

@qikaigao You still need to remove lines 483...487.

[qikaigao]

remove the undefined check at line 478 to 483

[qikaigao]

Sorry, I am new to PR. I just made the update

[ihrpr]

thank you @qikaigao. oh, didn't see it on time. it should be fixed with this PR

[cliffhall]

it should be fixed with this PR

Hey @ihrpr. Have a look at this implementation. The PR you pointed to does make the change that @qikaigao was initially making, but in the end, it was refactored to a more understandable reordering of the conditionals. It moves the check for stateless to the top of the function as an early return. The reader can then assume the rest of the logic is for a stateful connection. I would lobby for keeping @qikaigao's implementation.

[ihrpr]

Looks good, thank you! Would be good to add tests for this, but we can merge as is.

[ihrpr]

@qikaigao please can you update should reject requests to uninitialized server test? and then we can merge it

[ihrpr]

Hey @ihrpr. Have a look at this implementation. The PR you pointed to does make the change that @qikaigao was initially making, but in the end, it was refactored to a more understandable reordering of the conditionals. It moves the check for stateless to the top of the function as an early return. The reader can then assume the rest of the logic is for a stateful connection. I would lobby for keeping @qikaigao's implementation.

yeah, the current implementation actually solves a different problem. In multi-node deployment, not all servers will be initialized. A client can send an initialization request and verify capabilities on one node, but then start sending requests to a node that was not initialized.

PR that I mentioned solves the problem when multiple clients connect to a node.

[cliffhall]

PR that I mentioned solves the problem when multiple clients connect to a node.

@ihrpr I wasn't suggesting this as an alternate to the other PR; this part was only a slice of that other one's work.

I only meant that this section, the reordering of these two conditionals vs making one more complex and leaving the early return needlessly further down in the method, might be a good way for those particular lines to end up. For readability.

[ihrpr]

realised it should sessionIdGenerator not session id. Merged this commit and applied the change in #377

[cliffhall]

Nice one. If the generator is there, then definitely this is intended to be stateful, whereas lack of sessionId only infers such.

[cliffhall]

Closing. Fixed in #377