fix: SSE resource subscribe endpoint yielding raw dicts instead of SSE-formatted strings#3595
Conversation
ed74a15 to
fbc2efe
Compare
msureshkumar88
left a comment
There was a problem hiding this comment.
PR #3595 review complete. Recommendation: ✅ Approve
Summary
This PR fixes a critical SSE protocol compliance bug in the /resources/subscribe endpoint. The implementation is clean, well-tested, and maintains all security controls.
Key Findings
✅ Strengths:
- Correctly implements SSE format:
data: {json}\n\n - Comprehensive test coverage with new
test_subscribe_resource_events_sse_format - Maintains existing authentication/authorization via
@require_permission("resources.read") - Uses efficient
orjsonserialization - No security vulnerabilities introduced
…E-formatted strings Signed-off-by: Marek Dano <Marek.Dano@ibm.com>
Signed-off-by: Marek Dano <Marek.Dano@ibm.com>
Move the orjson import from inside test function to module-level imports (PEP 8 compliance). Align sse_generator() docstring with the existing generate_events() pattern used by the roots subscribe endpoint. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
fbc2efe to
5e08f2c
Compare
|
Rebased onto
No logic changes — the fix itself is correct and consistent with existing SSE patterns in the codebase. |
crivetimihai
left a comment
There was a problem hiding this comment.
LGTM. The fix is correct — resource_service.subscribe_events() yields raw dicts, and StreamingResponse needs strings. The SSE formatting pattern (data: {orjson.dumps(event).decode()}\n\n) is consistent with the roots subscribe endpoint and EventService.event_generator(). No security or performance concerns. Tests cover both the endpoint contract and the wire format.
…E-formatted strings (#3595) * fix: SSE resource subscribe endpoint yielding raw dicts instead of SSE-formatted strings Signed-off-by: Marek Dano <Marek.Dano@ibm.com> * fix: lint issue in main.py Signed-off-by: Marek Dano <Marek.Dano@ibm.com> * fix: move orjson import to module level and align docstring style Move the orjson import from inside test function to module-level imports (PEP 8 compliance). Align sse_generator() docstring with the existing generate_events() pattern used by the roots subscribe endpoint. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Marek Dano <Marek.Dano@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Marek Dano <Marek.Dano@ibm.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
…E-formatted strings (#3595) * fix: SSE resource subscribe endpoint yielding raw dicts instead of SSE-formatted strings Signed-off-by: Marek Dano <Marek.Dano@ibm.com> * fix: lint issue in main.py Signed-off-by: Marek Dano <Marek.Dano@ibm.com> * fix: move orjson import to module level and align docstring style Move the orjson import from inside test function to module-level imports (PEP 8 compliance). Align sse_generator() docstring with the existing generate_events() pattern used by the roots subscribe endpoint. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Marek Dano <Marek.Dano@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Marek Dano <Marek.Dano@ibm.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>
…E-formatted strings (IBM#3595) * fix: SSE resource subscribe endpoint yielding raw dicts instead of SSE-formatted strings Signed-off-by: Marek Dano <Marek.Dano@ibm.com> * fix: lint issue in main.py Signed-off-by: Marek Dano <Marek.Dano@ibm.com> * fix: move orjson import to module level and align docstring style Move the orjson import from inside test function to module-level imports (PEP 8 compliance). Align sse_generator() docstring with the existing generate_events() pattern used by the roots subscribe endpoint. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Marek Dano <Marek.Dano@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Marek Dano <Marek.Dano@ibm.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: KRISHNAN, SANTHANA <sk8069@exo.att.com>
🐛 Bug-fix PR
📌 Summary
Found this bug while executing manual tests on Resources #2419
The POST /resources/subscribe SSE endpoint at main.py:4864 passes resource_service.subscribe_events() directly to StreamingResponse. However, subscribe_events() yields raw Python dicts, while StreamingResponse expects an async generator of strings or bytes.
This causes the SSE connection to remain open (HTTP 200) but no events are ever written to the stream. Clients stay connected but never receive resource change notifications (resource_updated, resource_added, resource_deleted, etc.) over SSE.
🔁 Reproduction Steps
data: {"type":"resource_updated","data":{"id":"...","uri":"file:///config/settings.json",...},"timestamp":"..."}🐞 Root Cause
The
EventServicealready has a correctevent_generator()method that formats events asdata: {...}\n\n, but it was not being used by this endpoint.💡 Fix Description
Wrap the async generator in an SSE formatter that serializes each event dict to the standard SSE
data: {...}\n\nformat before yielding to StreamingResponse:This aligns with the pattern used by
EventService.event_generator()and the SSE specification (each event prefixed withdata:and terminated with\n\n).🧪 Verification
make lintmake testmake coverage📐 MCP Compliance (if relevant)
✅ Checklist
make black isort pre-commit)