Add SSE response mode to the 2026 streamable-HTTP server entry#3001
Conversation
The modern (2026-07-28) single-exchange entry could only respond with application/json, so a handler's ctx.report_progress() / ctx.log() was a no-op on that path. This adds the text/event-stream response mode: the handler runs as a sibling task inside an EventSourceResponse via data_sender_callable=, request-scoped notifications stream as SSE events before the terminal JSON-RPC response, and client disconnect cancels the handler (stream-close = cancellation per the transport spec). The existing json_response: bool on the session manager governs the modern path too — JSON mode keeps today's drop-notifications behaviour. Accept validation lands at the same time (mode-aware, matching the legacy entry; check_accept_headers hoisted to module level so both entries share it). progress_token_from_params() extracted to shared/ with the bool-guard. Burns the tools-call-with-progress conformance waiver, the _MODERN_NOTIFY_DROP KnownFailure rows, and the streaming story's http-asgi:modern xfail.
maxisbey
force-pushed
the
modern-sse-response
branch
from
7382d8e to
ea346df
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The previous commit committed text/event-stream headers immediately, so a METHOD_NOT_FOUND raised inside serve_one reached the wire as HTTP 200 + an SSE event — breaking the spec MUST that an unimplemented method respond 404. server-stateless conformance failed 7/25 on that. The handler now runs as a sibling task while the parent waits for the first event from the rendezvous stream. If the handler completes (or raises) without emitting, the result is written as application/json with the ERROR_CODE_HTTP_STATUS-mapped status — same as JSON mode. text/event-stream headers go out only once a notification has actually arrived; from there a disconnect-watcher in the same task group cancels serve_one on http.disconnect. EventSourceResponse is dropped for this path (it sends headers on __call__ and its task group can't adopt an already-running handler); SSE frames are written directly via ASGI send. Also: drop the _check_accept_headers wrapper method (review feedback).
The 5820a22 raw-ASGI rewrite dropped the comment-line keepalive that EventSourceResponse used to provide, so a long-silent handler behind a proxy idle-read timeout would have its stream closed (which on this path cancels the handler). The receive loop now emits ': ping' on a 15s move_on_after between events, matching the legacy paths' interval.
A handler that runs silent past _SSE_PING_INTERVAL now commits text/event-stream and starts pinging instead of leaving zero bytes on the wire — so a proxy idle-read timeout can't close the connection (which on this path cancels the handler) before the result arrives. Kernel-dispatch errors still get the table-mapped JSON status because they resolve well within the 15s window.
The two keepalive tests now run on trio's autojumping MockClock instead of a real 50ms anyio.sleep against a 10ms patched interval. The simulated 2.5s-silent handler takes no wall-clock time, the ping count is asserted exactly (== 2) instead of by presence, and there is no scheduler-timing window. Same pattern as test_jsonrpc_dispatcher.py and test_timeouts.py.
There was a problem hiding this comment.
I didn't find any new issues in the latest revision — the keepalive and Cache-Control concerns from my earlier inline comments look properly addressed (bounded first-event wait, ping loop, no-transform header) and are now covered by tests — but the hand-rolled ASGI SSE write path with deferred-start, disconnect-driven cancellation, and status-mapping semantics is complex enough that it warrants a human reviewer's sign-off.
Extended reasoning...
Overview
This PR adds an SSE response mode to the 2026-07-28 single-exchange streamable-HTTP server entry (_streamable_http_modern.py), so request-scoped progress and log notifications stream before the terminal response. It threads json_response through the manager, hoists check_accept_headers to module level in streamable_http.py, extracts progress_token_from_params into shared/jsonrpc_dispatcher.py, burns several conformance/interaction xfails, and adds a substantial unit-test suite for the new path (including trio MockClock keepalive timing tests and a disconnect-cancellation test).
Security risks
Low. The Accept-header validation is a byte-for-byte hoist of the existing parser (the q-value gap noted by another bot is pre-existing, not introduced here). No auth, crypto, or permission logic is touched; the new code only changes how a server's own response is framed on the wire. The notify path serializes JSON-RPC notifications it already constructs, so there is no new injection surface.
Level of scrutiny
High. This is production transport code on the default (json_response=False) path for the modern protocol revision, and the SSE branch is hand-rolled ASGI (deferred response start, a rendezvous memory stream, a sibling-task handler, disconnect-driven cancellation, and move_on_after-based keepalive). Earlier review iterations on this very PR surfaced two real robustness regressions (missing keepalive, unbounded pre-commit silence) that were only caught by careful reasoning about proxy idle timeouts — exactly the kind of subtle concurrency/deployment behavior that deserves a human maintainer's judgment on the final shape, including the deferred-start design tradeoff itself.
Other factors
All prior reviewer feedback (maxisbey's wrapper-removal comment and my inline findings on Cache-Control parity, the missing keepalive ping, and the unbounded first-event wait) has been addressed in follow-up commits with accompanying tests, and the current diff reflects those fixes. Test coverage of the new path is good: ordering, error-before/after-commit, 406 mode-aware Accept handling, wildcard Accept, late-notify drop, silent-handler commit, and disconnect cancellation are all exercised. I found no remaining bugs, but the size and criticality of the change put it outside what I'm willing to shadow-approve.
2 participants
The modern (2026-07-28) single-exchange entry could only respond with
application/json, so a handler'sctx.report_progress()/ request-scopednotifications/messagewas a no-op on that path. This adds thetext/event-streamresponse mode so those notifications stream before the terminal response.Part of #2893; tracked under #2891.
Motivation and Context
The transport spec says a server MAY respond to a POST with either
application/jsonortext/event-stream; the SSE stream carriesnotifications/progress/notifications/messagerelated to the originating request, then the terminal response, then closes. Stream-close is the cancellation signal. The legacy stack already does both modes viajson_response: bool; this is the modern entry catching up.How Has This Been Tested?
tests/server/test_streamable_http_modern.py: SSE streams progress + log in order before the result; JSON mode drops; handler error is a 200 +JSONRPCErrorevent; no-token → progress no-op; mode-aware 406 on Accept;*/*wildcard; late notify after stream-close drops; client disconnect cancels the handler andconnection.exit_stackstill unwinds.[streamable-http-2026-07-28]legs of the interaction-suite progress/logging requirements now pass.streamingexample story'shttp-asgi:modernleg now passes.tools-call-with-progressserver scenario fromexpected-failures.2026-07-28.yml.Breaking Changes
None for users — the manager's existing
json_responseflag governs the modern path too (defaultFalse= SSE, same as legacy). The modern entry now validatesAccept(mode-aware: JSON mode requiresapplication/json; SSE mode requires both), so a client that omittedAcceptentirely now gets 406 — but the spec already requires clients to send both.Types of changes
Checklist
Additional context
Shape:
_SingleExchangeDispatchContextgainsprogress_token+sink: MemoryObjectSendStream | None.notify()writes an SSE-framed event to the sink (catchingClosedResourceError/BrokenResourceError→ debug-log and drop, mirroring_JSONRPCDispatchContext); JSON mode hassink=Noneso it returns.progress()reads the token and callsnotify().send_raw_requeststill raisesNoBackChannelError— SSE is notify-only; sampling/elicitation/list-roots stay on the MRTR path.SSE write path — deferred start: the handler runs as a sibling task while the parent waits for the first event from a rendezvous (buffer=0) memory stream. If the handler completes (or raises) without emitting, the result is written as
application/jsonwith theERROR_CODE_HTTP_STATUS-mapped status — so the spec's404/400MUSTs hold for kernel-dispatch errors (METHOD_NOT_FOUND, missing-capability) even withjson_response=False.text/event-streamheaders go out only once a notification has actually arrived; from there awatch_disconnecttask in the same group cancelsserve_oneonhttp.disconnect(stream-close = cancellation), and amove_on_after(15)around the receive loop emits: pingcomment-line keepalives between events, matching the legacy paths' interval. SSE frames are written directly via ASGIsendrather thanEventSourceResponsebecause ESR sends headers on__call__and its internal task group can't adopt an already-running handler.async with send_ch:guarantees the stream closes on every exit path.Shared helpers:
check_accept_headers()hoisted to module level instreamable_http.py(the legacy method body never readself);progress_token_from_params()extracted toshared/jsonrpc_dispatcher.pywith the existing bool-guard. Both entries / both dispatch contexts call them.Coverage notes: the
streamingstory's modern-HTTP cancellation leg now exercises two pre-existing client-sideexcept Exceptionblocks inclient/streamable_http.py(forwarding an SSE event to aread_stream_writerthe cancel already closed). Promoted fromno cover→lax no coverwith an in-file comment; thelogger.exception("Error parsing SSE message")there is misleading (it'sBrokenResourceError, not a parse failure) — worth a follow-up but out of scope here.AI Disclaimer