-
Notifications
You must be signed in to change notification settings - Fork 1.6k
SEP-1699: Add SSE polling support with EventStore #2564
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
base: main
Are you sure you want to change the base?
Conversation
0387cf6 to
135e208
Compare
|
Warning Rate limit exceeded@jlowin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces SSE (Server-Sent Events) polling support for long-running operations with resumability. It adds a new EventStore class backed by an AsyncKeyValue storage backend to persist and replay events across SSE reconnections. The EventStore is integrated into the HTTP transport layer through new parameters ( Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/fastmcp/server/event_store.py (1)
116-131: Potential race condition in concurrent stream updates.The read-modify-write pattern on
stream_data.event_idsis not atomic. If twostore_eventcalls for the samestream_idexecute concurrently, one event could be lost when the second write overwrites the first.For single-stream-per-request scenarios (typical SSE usage), this is unlikely to cause issues. However, for shared streams or high-concurrency production deployments, consider documenting this limitation or adding optional locking.
If this becomes a concern in production, you could:
- Document that each stream should have a single writer
- Use Redis LPUSH/RPUSH for atomic list operations when using RedisStore
- Add an optional distributed lock wrapper for multi-writer scenarios
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/server/test_event_store.pyis excluded by none and included by none
📒 Files selected for processing (6)
docs/deployment/http.mdx(1 hunks)src/fastmcp/__init__.py(2 hunks)src/fastmcp/server/context.py(1 hunks)src/fastmcp/server/event_store.py(1 hunks)src/fastmcp/server/http.py(3 hunks)src/fastmcp/server/server.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Python source code must use Python ≥3.10 with full type annotations
Never use bareexcept- be specific with exception types
Prioritize readable, understandable code - clarity over cleverness; avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code organization and style
Files:
src/fastmcp/__init__.pysrc/fastmcp/server/context.pysrc/fastmcp/server/http.pysrc/fastmcp/server/server.pysrc/fastmcp/server/event_store.py
docs/**/*.mdx
📄 CodeRabbit inference engine (docs/.cursor/rules/mintlify.mdc)
docs/**/*.mdx: Use clear, direct language appropriate for technical audiences
Write in second person ('you') for instructions and procedures in MDX documentation
Use active voice over passive voice in MDX technical documentation
Employ present tense for current states and future tense for outcomes in MDX documentation
Maintain consistent terminology throughout all MDX documentation
Keep sentences concise while providing necessary context in MDX documentation
Use parallel structure in lists, headings, and procedures in MDX documentation
Lead with the most important information using inverted pyramid structure in MDX documentation
Use progressive disclosure in MDX documentation: present basic concepts before advanced ones
Break complex procedures into numbered steps in MDX documentation
Include prerequisites and context before instructions in MDX documentation
Provide expected outcomes for each major step in MDX documentation
End sections with next steps or related information in MDX documentation
Use descriptive, keyword-rich headings for navigation and SEO in MDX documentation
Focus on user goals and outcomes rather than system features in MDX documentation
Anticipate common questions and address them proactively in MDX documentation
Include troubleshooting for likely failure points in MDX documentation
Provide multiple pathways (beginner vs advanced) but offer an opinionated path to avoid overwhelming users in MDX documentation
Always include complete, runnable code examples that users can copy and execute in MDX documentation
Show proper error handling and edge case management in MDX code examples
Use realistic data instead of placeholder values in MDX code examples
Include expected outputs and results for verification in MDX code examples
Test all code examples thoroughly before publishing in MDX documentation
Specify language and include filename when relevant in MDX code examples
Add explanatory comments for complex logic in MDX code examples
Document all API...
Files:
docs/deployment/http.mdx
🧠 Learnings (1)
📚 Learning: 2025-12-04T00:17:41.238Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T00:17:41.238Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing using in-memory transport; only use HTTP transport when explicitly testing network features
Applied to files:
src/fastmcp/__init__.pydocs/deployment/http.mdxsrc/fastmcp/server/server.py
🧬 Code graph analysis (3)
src/fastmcp/__init__.py (1)
src/fastmcp/server/event_store.py (1)
EventStore(40-177)
src/fastmcp/server/server.py (2)
src/fastmcp/server/event_store.py (1)
EventStore(40-177)src/fastmcp/server/http.py (1)
StarletteWithLifespan(71-74)
src/fastmcp/server/event_store.py (1)
src/fastmcp/utilities/logging.py (1)
get_logger(14-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
🔇 Additional comments (10)
src/fastmcp/server/context.py (1)
489-526: LGTM! Well-documented SSE stream closing method.The implementation correctly handles the no-op case with a debug log when the transport doesn't support SSE polling. The condition on line 520 properly checks both that the request context exists and that it has a
close_sse_streamcallable. The docstring provides clear usage guidance and a practical example.src/fastmcp/__init__.py (1)
17-17: LGTM! EventStore correctly exposed in public API.The import and
__all__export follow existing patterns and maintain alphabetical ordering.Also applies to: 34-34
docs/deployment/http.mdx (2)
241-252: Clear workflow explanation.The 4-step workflow is well-documented and the note about
close_sse_stream()being a no-op without EventStore helps users understand the safe fallback behavior.
254-271: Good Redis backend example with configuration options.The custom storage backend section provides practical guidance for production deployments. The parameters (
max_events_per_stream,ttl) are documented inline.src/fastmcp/server/http.py (1)
274-315: LGTM!retry_intervalparameter correctly wired through.The new parameter is properly typed, documented, and forwarded to
StreamableHTTPSessionManager. The docstring correctly notes thatretry_intervalrequiresevent_storeto be set.Consider whether the SDK's
StreamableHTTPSessionManagervalidates thatretry_intervalis only meaningful whenevent_storeis provided, or if FastMCP should add a warning whenretry_intervalis set withoutevent_store.src/fastmcp/server/server.py (2)
67-67: LGTM! EventStore import correctly placed.
2343-2393: LGTM!http_appmethod correctly extended with SSE polling parameters.The
event_storeandretry_intervalparameters are:
- Properly typed with
EventStore | Noneandint | None- Well documented in the docstring with clear descriptions of their purpose
- Correctly passed only to
create_streamable_http_app(not to SSE transport, which doesn't support this feature)src/fastmcp/server/event_store.py (3)
26-37: LGTM! Clean Pydantic models for event storage.The
EventEntryandStreamEventListmodels are simple and well-structured. Usingdict | Nonefor the serialized message is appropriate for JSON-RPC message storage.
135-176: Solid replay implementation with good error handling.The method handles all edge cases gracefully:
- Missing event ID → warning log + return None
- Missing stream → warning log + return None
- Event ID not in stream list → warning log + return None
- Events without messages (priming events) are correctly skipped
70-92: LGTM! Well-designed constructor with sensible defaults.The constructor follows established patterns in the codebase (similar to ResponseCachingMiddleware and OAuthProxy). Default values are appropriate:
MemoryStorefor simple deployments- 100 events per stream limit
- 1-hour TTL
The
PydanticAdapterusage ensures type-safe serialization.
Test Failure AnalysisSummary: The integration test Root Cause: The test is calling the real GitHub Copilot MCP API endpoint ( This failure is unrelated to the PR's changes - the EventStore and SSE polling additions don't affect how this integration test connects to external APIs. This appears to be a transient rate limiting issue with GitHub's infrastructure. Suggested Solution:
Detailed AnalysisFrom the logs: The test successfully initiated the httpx.HTTPStatusError: Client error '429 Too Many Requests' for url 'https://api.githubcopilot.com/mcp/'
For more information check: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/429The 429 error indicates GitHub's API rate limiting kicked in. The test waited for 30+ seconds before pytest-timeout killed it. All other integration tests passed (13/14), suggesting this is a specific issue with the GitHub API's rate limits rather than a systemic problem. Related Files
|
Implements SEP-1699 SSE polling support for long-running operations.
When tools run for extended periods, load balancers often terminate idle connections. SSE polling allows the server to gracefully close connections and have clients automatically reconnect and resume from where they left off.
Changes:
EventStoreclass backed byAsyncKeyValuefor pluggable storage backendsContext.close_sse_stream()for server-initiated disconnectionevent_storeandretry_intervalparameters onhttp_app()Requires MCP SDK with SEP-1699 support (modelcontextprotocol/python-sdk#1654).