-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: report_progress missing passing related_request_id causes notifications not working in streaming-http #838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: report_progress missing passing related_request_id causes notifications not working in streaming-http #838
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses an issue in the report_progress functionality by ensuring that the related_request_id is passed along, which fixes notifications not working in stateless_http mode. Key changes include:
- Adding the related_request_id parameter to the report_progress call in the server context.
- Updating server initialization and test fixtures to support the stateless_http mode configuration.
- Introducing a new parameterized pytest for testing the greet tool progress reporting.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
tests/client/test_streamable_http.py | Updated test fixtures and added tool testing for progress reporting. |
src/fastmcp/server/context.py | Modified report_progress to include related_request_id in the payload. |
Comments suppressed due to low confidence (2)
src/fastmcp/server/context.py:125
- Ensure that adding 'related_request_id' to the progress payload does not affect progress handler consumers. Verify that all clients or progress handlers are updated to either handle or ignore this additional parameter as necessary.
related_request_id=self.request_id,
tests/client/test_streamable_http.py:145
- The test asserts the progress handler receives three parameters, but the updated implementation now passes an extra 'related_request_id'. Adjust the test assertion or confirm that the extra parameter is properly handled (or ignored) in progress handler invocation.
progress_handler.assert_called_once_with(0.5, 1.0, "Greeting in progress")
Thank you! |
I wonder why the low-level doesn't fill that in by default 🤔 But appreciate the fix! |
Thats a very good question and if the server is run in stateful mode, the requests are handled differently through the For other logging methods, the
For the context class in FastMCP (in this repo), these are also missing as I see. What I'm not entirely sure is if there are any side effects that I may have introduced due to this change. |
…s-missing-related-request-id fix: report_progress missing passing related_request_id causes notifications not working in streaming-http
This pull request fixes an issue related to
report_progress
that misses passing therelated_request_id
which causes notifications not working instateless_http=True
mode.This issue was also reported for the FastMCP implementation in the Python MCP SDK: modelcontextprotocol/python-sdk#953
I also added a test for
stateless_http=True
mode introducing a parameterized pytest (not sure if you prefer having this in a separate file or keep it there).